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.)
<rdar://problem/40355211>
Created attachment 340676 [details] Patch v1
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
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 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 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.)
Created attachment 340863 [details] Patch v2
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 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 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.
Created attachment 340952 [details] Patch v3
(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 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.
(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.
Created attachment 341101 [details] Patch v4
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.
Committed r232139: <https://trac.webkit.org/changeset/232139>