Bug 185757 – Don't create the SubimageCache just to clear an image from it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

When a NativeImageCG object is destroyed, it makes sure there are no references to it left in the subimage cache if the CACHE_SUBIMAGES macro is defined.

However, clearing of the subimage cache actually creates it first (if it doesn't exist yet) just to make sure the image isn't in the cache!.  This isn't very efficient, especially in the UI Process where it is rarely--if ever--used.

Instead, we can return early when clearing the subimage cache if it doesn't exist yet, thus preventing extra work and extra memory allocated for it.

This also makes it possible for us to land a fix for Bug 185330 to check to see if there are other uses of WebCore::Timer in the UI Process.  (The SubimageCacheWithTimer class uses a DeferrableOneShotTimer, which is a subclass of WebCore::TimerBase.)

[Radar WebKit Bug Importer]

<rdar://problem/40355211>

[David Kilzer (:ddkilzer)]

Created attachment 340676 [details]
Patch v1

[EWS Watchlist]

Comment on attachment 340676 [details]
Patch v1

Attachment 340676 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7722572

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 340700 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tim Horton]

Comment on attachment 340676 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=340676&action=review

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp:38
>  
> +SubimageCacheWithTimer* SubimageCacheWithTimer::s_cache;
> +std::once_flag SubimageCacheWithTimer::s_onceFlag;

Why are these here and in the header?

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp:119
> +    std::call_once(SubimageCacheWithTimer::s_onceFlag, [] {

Why? This is WebCore code that’s main-thread-only, we can just null-check.

[David Kilzer (:ddkilzer)]

Comment on attachment 340676 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=340676&action=review

>> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp:38
>> +std::once_flag SubimageCacheWithTimer::s_onceFlag;
> 
> Why are these here and in the header?

They are 'static' class variables, so they're declared within the class in the header, but space is allocated (just once) in the *.cpp file.

>> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.cpp:119
>> +    std::call_once(SubimageCacheWithTimer::s_onceFlag, [] {
> 
> Why? This is WebCore code that’s main-thread-only, we can just null-check.

Wil fix.  Wasn't sure if breaking this out from the WebCore::Image constructor could cause any issues with it being called from multiple threads.  (I guess if that was the case, I'd need locking in ::cacheExists() as well.)

[David Kilzer (:ddkilzer)]

Created attachment 340863 [details]
Patch v2

[Simon Fraser (smfr)]

Comment on attachment 340863 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=340863&action=review

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:89
> +    static bool cacheExists();
> +    static SubimageCacheWithTimer& getCache();

Usually we have getCache() and getExistingCache(), which I prefer.

[Said Abou-Hallawa]

Comment on attachment 340863 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=340863&action=review

> Source/WebCore/ChangeLog:14
> +        helper functions in a namespade (for syntactic sugar) that

namespace?

>> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:89
>> +    static SubimageCacheWithTimer& getCache();
> 
> Usually we have getCache() and getExistingCache(), which I prefer.

Why do we use the verb 'get' in the names? Should not they be cache() and existingCache()? The original function's name was subimageCache().

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:119
> +namespace SubimageCache {
> +
> +RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
> +void clearImage(CGImageRef);
> +
> +}
> +
> +inline RetainPtr<CGImageRef> SubimageCache::getSubimage(CGImageRef image, const FloatRect& rect)
> +{
> +    return SubimageCacheWithTimer::getCache().getSubimage(image, rect);
> +}
> +
> +inline void SubimageCache::clearImage(CGImageRef image)
> +{
> +    if (!SubimageCacheWithTimer::cacheExists())
> +        return;
> +    SubimageCacheWithTimer::getCache().clearImage(image);
> +}

I am not sure I like using namespace in this way. Can not these two functions be static functions of SubimageCacheWithTimer?

If they are static functions of SubimageCacheWithTimer, there is no need to make getCache() or cacheExists() public.

[David Kilzer (:ddkilzer)]

Comment on attachment 340863 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=340863&action=review

>> Source/WebCore/ChangeLog:14
>> +        helper functions in a namespade (for syntactic sugar) that
> 
> namespace?

Will fix.

>>> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:89
>>> +    static SubimageCacheWithTimer& getCache();
>> 
>> Usually we have getCache() and getExistingCache(), which I prefer.
> 
> Why do we use the verb 'get' in the names? Should not they be cache() and existingCache()? The original function's name was subimageCache().

I guess this question was for Simon?

BTW, if we switched semantics to getExistingCache(), the function would have to return a pointer, and then the null check would move to client code, which means clients would need to know to null-check before calling clearImage(), which is more error-prone.  I wanted to make it easy to continue to use a clearImage() function that just does the right thing if the cache doesn't exist (by returning early), so I'll just switch to moving clearImage() into a static method on SubimageCacheWithTimer.

>> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:119
>> +}
> 
> I am not sure I like using namespace in this way. Can not these two functions be static functions of SubimageCacheWithTimer?
> 
> If they are static functions of SubimageCacheWithTimer, there is no need to make getCache() or cacheExists() public.

I mentioned this in the ChangeLog:

Finally, we provide two inline helper functions in a namespace (for syntactic sugar) that provide easy access to the subimage cache and abstract the implementation details a bit:  SubimageCache::getSubimage() and SubimageCache::clearImage().

Old way:
    subImage = subimageCache().getSubimage(subImage.get(), subimageRect);
    subimageCache().clearImage(image.get());

New way:
    subImage = SubimageCache::getSubimage(subImage.get(), subimageRect);
    SubimageCache::clearImage(image.get());

I'll move them to static methods on SubimageCacheWithTimer instead.

[David Kilzer (:ddkilzer)]

Created attachment 340952 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #11)
> Created attachment 340952 [details]
> Patch v3

Old way:
    subImage = subimageCache().getSubimage(subImage.get(), subimageRect);
    subimageCache().clearImage(image.get());

New way:
    subImage = SubimageCacheWithTimer::getSubimage(subImage.get(), subimageRect);
    SubimageCacheWithTimer::clearImage(image.get());

Changing these to static class functions did let me remove the other helper methods, at the expense of the longer "SubimageCacheWithTimer" class name and sprinkling an instance variable throughout getSubimage() and clearImage().

[Said Abou-Hallawa]

Comment on attachment 340952 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=340952&action=review

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:86
> -public:
> -    SubimageCacheWithTimer();
> -    RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
> -    void clearImage(CGImageRef);
> +    static RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
> +    static void clearImage(CGImageRef);

I am very sorry. I think my last comments were not very clear. What I meant is adding two new static functions for getting the sub image and clearing the image and making the existing two functions be private with no change. To be sure I stated what I meant correctly this time, here is what I meant in terms of code:

class SubimageCacheWithTimer
{
public:
    static RetainPtr<CGImageRef> getSubimage(CGImageRef image, const FloatRect& rect)
    {
        return subimageCache().subImage(image, rect);
    }

    static void clearImage(CGImageRef image)
    {
        if (subimageCacheExists())
            subimageCache().clear(image)
    }

private:
    static SubimageCacheWithTimer& subimageCache()
    {
        if (!s_subimageCache)
            s_subimageCache = new SubimageCacheWithTimer;
       return *s_subimageCache;
    }

    static bool subimageCacheExists() { return s_subimageCache; }

    RetainPtr<CGImageRef> subImage(CGImageRef, const FloatRect&);
    void clear(CGImageRef);
}

And in the cpp file, SubimageCacheWithTimer::getSubimage() will be renamed to SubimageCacheWithTimer::subImage() and SubimageCacheWithTimer::clearImage() will be renamed to SubimageCacheWithTimer::clear() or any other better names.

[David Kilzer (:ddkilzer)]

(In reply to Said Abou-Hallawa from comment #13)
> Comment on attachment 340952 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340952&action=review
> 
> > Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:86
> > -public:
> > -    SubimageCacheWithTimer();
> > -    RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
> > -    void clearImage(CGImageRef);
> > +    static RetainPtr<CGImageRef> getSubimage(CGImageRef, const FloatRect&);
> > +    static void clearImage(CGImageRef);
> 
> I am very sorry. I think my last comments were not very clear. What I meant
> is adding two new static functions for getting the sub image and clearing
> the image and making the existing two functions be private with no change.
> To be sure I stated what I meant correctly this time, here is what I meant
> in terms of code:

Okay, thanks for the clarification!  I will implement this.

[David Kilzer (:ddkilzer)]

Created attachment 341101 [details]
Patch v4

[Said Abou-Hallawa]

Comment on attachment 341101 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=341101&action=review

> Source/WebCore/platform/graphics/cg/SubimageCacheWithTimer.h:83
> -    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCache;
> +    typedef HashSet<SubimageCacheEntry, SubimageCacheHash, SubimageCacheEntryTraits> SubimageCacheHashSet;

Nit: I think this typedef can be private also.

[David Kilzer (:ddkilzer)]

Committed r232139: <https://trac.webkit.org/changeset/232139>