WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
115902
Caching of generated images in CSS should be smarter.
https://bugs.webkit.org/show_bug.cgi?id=115902
Summary
Caching of generated images in CSS should be smarter.
Andreas Kling
Reported
2013-05-10 05:52:33 PDT
<
rdar://problem/13542727
>
Attachments
Possibly a patch
(19.71 KB, patch)
2013-05-10 06:39 PDT
,
Andreas Kling
eflews.bot
: commit-queue-
Details
Formatted Diff
Diff
Probably a patch
(16.99 KB, patch)
2013-05-10 08:58 PDT
,
Andreas Kling
koivisto
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2013-05-10 06:39:43 PDT
Created
attachment 201341
[details]
Possibly a patch
EFL EWS Bot
Comment 2
2013-05-10 06:43:37 PDT
Comment on
attachment 201341
[details]
Possibly a patch
Attachment 201341
[details]
did not pass efl-wk2-ews (efl-wk2): Output:
http://webkit-queues.appspot.com/results/321065
EFL EWS Bot
Comment 3
2013-05-10 06:44:39 PDT
Comment on
attachment 201341
[details]
Possibly a patch
Attachment 201341
[details]
did not pass efl-ews (efl): Output:
http://webkit-queues.appspot.com/results/265092
Early Warning System Bot
Comment 4
2013-05-10 06:45:01 PDT
Comment on
attachment 201341
[details]
Possibly a patch
Attachment 201341
[details]
did not pass qt-ews (qt): Output:
http://webkit-queues.appspot.com/results/456021
Early Warning System Bot
Comment 5
2013-05-10 06:45:30 PDT
Comment on
attachment 201341
[details]
Possibly a patch
Attachment 201341
[details]
did not pass qt-wk2-ews (qt-wk2): Output:
http://webkit-queues.appspot.com/results/427091
Antti Koivisto
Comment 6
2013-05-10 06:56:04 PDT
Comment on
attachment 201341
[details]
Possibly a patch View in context:
https://bugs.webkit.org/attachment.cgi?id=201341&action=review
> Source/WebCore/css/CSSImageGeneratorValue.cpp:64 > + auto it = m_clients.find(renderer);
Hi Anders!
> Source/WebCore/css/CSSImageGeneratorValue.cpp:82 > + GeneratedImageCache* cache = m_clients.get(renderer); > + if (!cache) > + return 0; > + > + CachedGeneratedImage* cachedGeneratedImage = cache->images.get(size); > + if (!cachedGeneratedImage) > + return 0;
If I understand correctly the cache entries can no longer be shared between multiple (same sized) renderers.Could this be a problem? Is there are solution that would keep this property?
kov's GTK+ EWS bot
Comment 7
2013-05-10 08:57:43 PDT
Comment on
attachment 201341
[details]
Possibly a patch
Attachment 201341
[details]
did not pass gtk-ews (gtk): Output:
http://webkit-queues.appspot.com/results/276232
Andreas Kling
Comment 8
2013-05-10 08:58:57 PDT
Created
attachment 201354
[details]
Probably a patch
Andreas Kling
Comment 9
2013-05-10 09:45:19 PDT
(In reply to
comment #6
)
> (From update of
attachment 201341
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=201341&action=review
> > > Source/WebCore/css/CSSImageGeneratorValue.cpp:82 > > + GeneratedImageCache* cache = m_clients.get(renderer); > > + if (!cache) > > + return 0; > > + > > + CachedGeneratedImage* cachedGeneratedImage = cache->images.get(size); > > + if (!cachedGeneratedImage) > > + return 0; > > If I understand correctly the cache entries can no longer be shared between multiple (same sized) renderers.Could this be a problem? Is there are solution that would keep this property?
Having per-RenderObject caches was never a requirement in the first place, I simplified things in the new patch by making the cache a simple IntSize=>GeneratorGeneratedImage map.
Antti Koivisto
Comment 10
2013-05-10 10:15:21 PDT
Comment on
attachment 201354
[details]
Probably a patch View in context:
https://bugs.webkit.org/attachment.cgi?id=201354&action=review
r=me, looks so much better.
> Source/WebCore/css/CSSImageGeneratorValue.cpp:38 > +static const double timeToKeepCachedGeneratedImagesInSeconds = 3;
Is there science behind this number?
> Source/WebCore/css/CSSImageGeneratorValue.cpp:100 > +void CSSImageGeneratorValue::CachedGeneratedImage::evictionTimerFired(DeferrableOneShotTimer<CachedGeneratedImage>*) > { > - m_images.add(size, image); > + m_owner.evictCachedGeneratedImage(m_size);
This is essentially "delete this" right? It would be good to add a comment pointing out that the object is dead afterwards.
Andreas Kling
Comment 11
2013-05-10 10:34:07 PDT
Committed
r149886
: <
http://trac.webkit.org/changeset/149886
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug