Bug 185757 - Don't create the SubimageCache just to clear an image from it
Summary: Don't create the SubimageCache just to clear an image from it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 185330
  Show dependency treegraph
 
Reported: 2018-05-17 20:54 PDT by David Kilzer (:ddkilzer)
Modified: 2018-05-23 17:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (8.08 KB, patch)
2018-05-17 20:59 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.79 MB, application/zip)
2018-05-18 09:45 PDT, Build Bot
no flags Details
Patch v2 (7.94 KB, patch)
2018-05-21 10:50 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (7.60 KB, patch)
2018-05-21 21:53 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v4 (9.55 KB, patch)
2018-05-23 10:18 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2018-05-17 20:54:20 PDT
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.)
Comment 1 Radar WebKit Bug Importer 2018-05-17 20:54:39 PDT
<rdar://problem/40355211>
Comment 2 David Kilzer (:ddkilzer) 2018-05-17 20:59:46 PDT
Created attachment 340676 [details]
Patch v1
Comment 3 Build Bot 2018-05-18 09:45:18 PDT
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
Comment 4 Build Bot 2018-05-18 09:45:29 PDT
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
Comment 5 Tim Horton 2018-05-18 12:18:48 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2018-05-21 10:44:52 PDT
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.)
Comment 7 David Kilzer (:ddkilzer) 2018-05-21 10:50:47 PDT
Created attachment 340863 [details]
Patch v2
Comment 8 Simon Fraser (smfr) 2018-05-21 11:05:28 PDT
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.
Comment 9 Said Abou-Hallawa 2018-05-21 12:38:01 PDT
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.
Comment 10 David Kilzer (:ddkilzer) 2018-05-21 15:15:11 PDT
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.
Comment 11 David Kilzer (:ddkilzer) 2018-05-21 21:53:33 PDT
Created attachment 340952 [details]
Patch v3
Comment 12 David Kilzer (:ddkilzer) 2018-05-22 04:52:06 PDT
(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().
Comment 13 Said Abou-Hallawa 2018-05-22 12:48:12 PDT
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.
Comment 14 David Kilzer (:ddkilzer) 2018-05-23 09:17:13 PDT
(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.
Comment 15 David Kilzer (:ddkilzer) 2018-05-23 10:18:21 PDT
Created attachment 341101 [details]
Patch v4
Comment 16 Said Abou-Hallawa 2018-05-23 11:03:35 PDT
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.
Comment 17 David Kilzer (:ddkilzer) 2018-05-23 17:50:42 PDT
Committed r232139: <https://trac.webkit.org/changeset/232139>