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
Attachments
Possibly a patch (19.71 KB, patch)
2013-05-10 06:39 PDT, Andreas Kling
eflews.bot: commit-queue-
Probably a patch (16.99 KB, patch)
2013-05-10 08:58 PDT, Andreas Kling
koivisto: review+
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
Note You need to log in before you can comment on or make changes to this bug.