Bug 115902 – Caching of generated images in CSS should be smarter.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

<rdar://problem/13542727>

[Andreas Kling]

Created attachment 201341 [details]
Possibly a patch

[EFL EWS Bot]

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 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 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 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 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 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]

Created attachment 201354 [details]
Probably a patch

[Andreas Kling]

(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 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]

Committed r149886: <http://trac.webkit.org/changeset/149886>