Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
286273002:
Optimize CSS box-shadow performance by caching the SkMask of the blur effect. (Closed)
|
Created:
6 years, 7 months ago by Jun Jiang Modified:
5 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionOptimize CSS box-shadow performance by caching the SkMask of the blur effect.
BUG=173807
Patch Set 1 #
Total comments: 5
Patch Set 2 : add mutex for thread-safe and use SkDiscardableMemory #
Total comments: 5
Patch Set 3 : re-use SkScaledImageCache and remove memory limit #Patch Set 4 : add changes to avoid subclassing of SkMask #Patch Set 5 : rebase with latest skia in git instead of svn trunk #
Total comments: 7
Messages
Total messages: 32 (0 generated)
Sorry for the delay. The network is not quite stable today and most time was spent on downloading skia code. Please help to take a look. Thanks.
Just some general comments from the pov of how this will impact chromium. Skia folks can comment on the actual implementation. https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... src/effects/SkBlurMaskFilter.cpp:166: SkBlurMaskFilterImpl::BlurMaskRecordList SkBlurMaskFilterImpl::fCachedBlurMaskRecordList; I'm failing to see how the use of this is thread safe. Does it need to be protected by a mutex? https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... src/effects/SkBlurMaskFilter.cpp:169: unsigned SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; Please allow SkDiscardableMemory to be used instead. Something similar to SkScaledImageCache as we don't want another cache that adds to chromium's total amount of memory.
Hi, reveman. Thanks for your comments and suggestions. https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... src/effects/SkBlurMaskFilter.cpp:166: SkBlurMaskFilterImpl::BlurMaskRecordList SkBlurMaskFilterImpl::fCachedBlurMaskRecordList; On 2014/05/16 18:49:16, reveman wrote: > I'm failing to see how the use of this is thread safe. Does it need to be > protected by a mutex? Yes, I should add a mutex to make it thread-safe. Will make the change in updated CL. https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... src/effects/SkBlurMaskFilter.cpp:169: unsigned SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; On 2014/05/16 18:49:16, reveman wrote: > Please allow SkDiscardableMemory to be used instead. Something similar to > SkScaledImageCache as we don't want another cache that adds to chromium's total > amount of memory. I have had a look at the interface of SkScaledImageCache and it should be feasible to adopt similar changes. One issue is that, I found SkDiscardableMemory is not enabled for Image decoder case for Android platform. Will SkDiscardableMemory be enabled and used for all platforms ? Need to hear more opinions on this.
https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/1/src/effects/SkBlurMaskFilter... src/effects/SkBlurMaskFilter.cpp:169: unsigned SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; On 2014/05/19 12:54:49, junj wrote: > On 2014/05/16 18:49:16, reveman wrote: > > Please allow SkDiscardableMemory to be used instead. Something similar to > > SkScaledImageCache as we don't want another cache that adds to chromium's > total > > amount of memory. > > I have had a look at the interface of SkScaledImageCache and it should be > feasible to adopt similar changes. > > One issue is that, I found SkDiscardableMemory is not enabled for Image decoder > case for Android platform. Will SkDiscardableMemory be enabled and used for all > platforms ? Need to hear more opinions on this. Yes, I'm hoping to enable it this week and remove all the old non-discardable memory cache code.
I have added mutex for thread-safe access and used SkDiscardableMemory as suggested. Verified on Linux and Android 4.4(Nexus 7)for performance boost. Please help to take another look. BTW, I have moved SkDiscardableMemory.h to include/core and keep the original one at src/core and not deleted because in chromium, skia/ext/SkDiscardableMemory_chrome.h has some code snippets hard-coded to "third_party/skia/src/code/SkDiscardableMemory.h".
Add more people in the loop for review.
https://codereview.chromium.org/286273002/diff/20001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (left): https://codereview.chromium.org/286273002/diff/20001/src/core/SkMaskFilter.cp... src/core/SkMaskFilter.cpp:226: SkMask::FreeImage(patch.fMask.fImage); NinePatch doesn't inherit from BlurMaskRecord, so you aren't getting this deletion from the destructor, right?
adding weight to SkMask is tricky, as we use that for text blits as well. I like the notion of a cache of ninepatchblurs. The plumbing technique here is not a good fit for skia, but it is motivating. I will think about how it might be approached to more easily fit into skia.
@tomhudson, thanks for your comments. https://codereview.chromium.org/286273002/diff/20001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (left): https://codereview.chromium.org/286273002/diff/20001/src/core/SkMaskFilter.cp... src/core/SkMaskFilter.cpp:226: SkMask::FreeImage(patch.fMask.fImage); On 2014/05/22 10:01:50, tomhudson wrote: > NinePatch doesn't inherit from BlurMaskRecord, so you aren't getting this > deletion from the destructor, right? Currently NinePatch and BlurMaskRecord are two independent structures. The only connection is that the SkMask data member in each of them points to the same Image memory. Since the idea is to cache the blured SkMask, so the Image memory is only freed when the BlurMaskRecord is destructed. For NinePatch, it either reuse the cached Image memory in BlurMaskRecord or generate the Image memory and then cache it in a BlurMaskRecord,
On 2014/05/22 19:16:52, reed1 wrote: > adding weight to SkMask is tricky, as we use that for text blits as well. I like > the notion of a cache of ninepatchblurs. The plumbing technique here is not a > good fit for skia, but it is motivating. I will think about how it might be > approached to more easily fit into skia. Yes, I also think it is a little tricky to introduce SkDiscardableMemory into SkMask. One motivation to do this way is that it is much simpler and can have least change to use SkDiscardableMemory for our case. If you have any big picture or idea in mind, I'd like to adjust the patch to align with your idea.
Just some discardable memory related comments. I'm not the right person to comment on the details of the implementation but looks like this approach will duplicate an unsatisfying amount of the scaled image cache code. I'm sure Mike and the rest of the skia folks can come up with some good ideas for how this can be properly done though. https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:160: typedef std::list<BlurMaskRecord*> BlurMaskRecordList; Probably need a hash to make sure this scales well.. https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:173: unsigned SkBlurMaskFilterImpl::fMaxBlurMaskListSize = 50; This limit could be much higher when using SkDiscardableMemory as it's only used to limit the number of DM instances we keep track of and not how memory is used. https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:174: unsigned SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; We don't want this limit when using SkDiscardableMemory.
On 2014/05/23 02:31:29, reveman wrote: > Just some discardable memory related comments. > > I'm not the right person to comment on the details of the implementation but > looks like this approach will duplicate an unsatisfying amount of the scaled > image cache code. I'm sure Mike and the rest of the skia folks can come up with > some good ideas for how this can be properly done though. > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > File src/effects/SkBlurMaskFilter.cpp (right): > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:160: typedef std::list<BlurMaskRecord*> > BlurMaskRecordList; > Probably need a hash to make sure this scales well.. > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:173: unsigned > SkBlurMaskFilterImpl::fMaxBlurMaskListSize = 50; > This limit could be much higher when using SkDiscardableMemory as it's only used > to limit the number of DM instances we keep track of and not how memory is used. > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:174: unsigned > SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; > We don't want this limit when using SkDiscardableMemory. Yes, I agree there is some duplicate code with scaled image cache code. All of the related functions accept the type of SkMask and carry out calculations based on SkMask type, thus we had to use SkDiacardableMemory interface and similar things with that in scaled image cache code. There is one alternative to bypass this limitation. Instead of using SkMask in NinePatch directly, we introduce SkBitmap into NinePatch instead. That is, the definition of NinePatch would be like this: struct NinePatch { SkBitmap fBitmap; // hold the storage. SkIRect fBounds; // hold the offset and size. SkIRect fOuterRect; SkIPoint fCenter; }; In short, fBitmap + fBounds = fMask. And we pass these two parameters down(fBitmap, fBounds) for calculation and can use the existing scaled image cache code directly. The code change is still small. @reed, @reveman, what's your opinion on this approach?
On 2014/05/26 04:28:47, Jun Jiang wrote: > On 2014/05/23 02:31:29, reveman wrote: > > Just some discardable memory related comments. > > > > I'm not the right person to comment on the details of the implementation but > > looks like this approach will duplicate an unsatisfying amount of the scaled > > image cache code. I'm sure Mike and the rest of the skia folks can come up > with > > some good ideas for how this can be properly done though. > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > File src/effects/SkBlurMaskFilter.cpp (right): > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > src/effects/SkBlurMaskFilter.cpp:160: typedef std::list<BlurMaskRecord*> > > BlurMaskRecordList; > > Probably need a hash to make sure this scales well.. > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > src/effects/SkBlurMaskFilter.cpp:173: unsigned > > SkBlurMaskFilterImpl::fMaxBlurMaskListSize = 50; > > This limit could be much higher when using SkDiscardableMemory as it's only > used > > to limit the number of DM instances we keep track of and not how memory is > used. > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > src/effects/SkBlurMaskFilter.cpp:174: unsigned > > SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; > > We don't want this limit when using SkDiscardableMemory. > > Yes, I agree there is some duplicate code with scaled image cache code. All of > the related functions accept the type of SkMask and carry out calculations based > on SkMask type, thus we had to use SkDiacardableMemory interface and similar > things with that in scaled > image cache code. > > There is one alternative to bypass this limitation. Instead of using SkMask in > NinePatch directly, we introduce SkBitmap into NinePatch instead. That is, the > definition of NinePatch would be like this: > struct NinePatch { > SkBitmap fBitmap; // hold the storage. > SkIRect fBounds; // hold the offset and size. > SkIRect fOuterRect; > SkIPoint fCenter; > }; > In short, fBitmap + fBounds = fMask. And we pass these two parameters > down(fBitmap, fBounds) for calculation and can use the existing scaled image > cache code directly. The code change is still small. > > @reed, @reveman, what's your opinion on this approach? Certainly I'd like to try reusing the scaledimagecache. However, we are in the midst of "upgarding" it to address a different bug, so its api may be in temporary flux. Either way, try our your idea in a new CL and we can evaluate it.
On 2014/05/27 13:42:33, reed1 wrote: > On 2014/05/26 04:28:47, Jun Jiang wrote: > > On 2014/05/23 02:31:29, reveman wrote: > > > Just some discardable memory related comments. > > > > > > I'm not the right person to comment on the details of the implementation but > > > looks like this approach will duplicate an unsatisfying amount of the scaled > > > image cache code. I'm sure Mike and the rest of the skia folks can come up > > with > > > some good ideas for how this can be properly done though. > > > > > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > > File src/effects/SkBlurMaskFilter.cpp (right): > > > > > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > > src/effects/SkBlurMaskFilter.cpp:160: typedef std::list<BlurMaskRecord*> > > > BlurMaskRecordList; > > > Probably need a hash to make sure this scales well.. > > > > > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > > src/effects/SkBlurMaskFilter.cpp:173: unsigned > > > SkBlurMaskFilterImpl::fMaxBlurMaskListSize = 50; > > > This limit could be much higher when using SkDiscardableMemory as it's only > > used > > > to limit the number of DM instances we keep track of and not how memory is > > used. > > > > > > > > > https://codereview.chromium.org/286273002/diff/20001/src/effects/SkBlurMaskFi... > > > src/effects/SkBlurMaskFilter.cpp:174: unsigned > > > SkBlurMaskFilterImpl::fMaxImageMemorySize = 2 * 1024 * 1024; > > > We don't want this limit when using SkDiscardableMemory. > > > > Yes, I agree there is some duplicate code with scaled image cache code. All of > > the related functions accept the type of SkMask and carry out calculations > based > > on SkMask type, thus we had to use SkDiacardableMemory interface and similar > > things with that in scaled > > image cache code. > > > > There is one alternative to bypass this limitation. Instead of using SkMask in > > NinePatch directly, we introduce SkBitmap into NinePatch instead. That is, the > > definition of NinePatch would be like this: > > struct NinePatch { > > SkBitmap fBitmap; // hold the storage. > > SkIRect fBounds; // hold the offset and size. > > SkIRect fOuterRect; > > SkIPoint fCenter; > > }; > > In short, fBitmap + fBounds = fMask. And we pass these two parameters > > down(fBitmap, fBounds) for calculation and can use the existing scaled image > > cache code directly. The code change is still small. > > > > @reed, @reveman, what's your opinion on this approach? > > Certainly I'd like to try reusing the scaledimagecache. However, we are in the > midst of "upgarding" it to address a different bug, so its api may be in > temporary flux. Either way, try our your idea in a new CL and we can evaluate > it. @reed and @reveman, thanks for your comments. Sorry for my delay to update the CL since I was in traveling last week. In my new CL, I had re-used SkScaledImageCache and removed the original memory limit code. It is much clean and simpler now. And it should have minimal API change. Please help to take another look.
tried to apply locally, and failed to merge. can you rebase? Off hand, the virtual on SkMask makes me nervous, as that would stop us from being able to make copies of masks, as we do with bitmaps today. We work very hard to not allow subclassing of bitmaps (and masks).
On 2014/06/04 15:14:38, reed1 wrote: > tried to apply locally, and failed to merge. can you rebase? > > Off hand, the virtual on SkMask makes me nervous, as that would stop us from > being able to make copies of masks, as we do with bitmaps today. We work very > hard to not allow subclassing of bitmaps (and masks). @reed, thanks for your quick reply and comments. It seems to me the CL is already rebased against latest code. The last commit below is as following: commit a4bb4cf7c401d9f0aed34ab0f26cdce2d1336045 Author: borenet@google.com <borenet@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81> Date: Mon Jun 2 11:48:44 2014 +0000 Whitespace change to verify that git_merger is off Review URL: https://codereview.chromium.org/312523002 git-svn-id: https://skia.googlecode.com/svn/trunk@15017 2bbb7eff-a529-9590-31e7-b0007b416f81 As for the virtual functions added to SkMask, I will consider to remove it. My idea is to let SkDiscardableMemoryMask inherit SkMask simply to avoid possible API change, such as filterMask(...), etc.
On 2014/06/04 15:28:09, junj wrote: > On 2014/06/04 15:14:38, reed1 wrote: > > tried to apply locally, and failed to merge. can you rebase? > > > > Off hand, the virtual on SkMask makes me nervous, as that would stop us from > > being able to make copies of masks, as we do with bitmaps today. We work very > > hard to not allow subclassing of bitmaps (and masks). > > @reed, thanks for your quick reply and comments. > > It seems to me the CL is already rebased against latest code. The last commit > below is as following: > commit a4bb4cf7c401d9f0aed34ab0f26cdce2d1336045 > Author: mailto:borenet@google.com > <mailto:borenet@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81> > Date: Mon Jun 2 11:48:44 2014 +0000 > > Whitespace change to verify that git_merger is off > > Review URL: https://codereview.chromium.org/312523002 > > git-svn-id: https://skia.googlecode.com/svn/trunk@15017 > 2bbb7eff-a529-9590-31e7-b0007b416f81 > > As for the virtual functions added to SkMask, I will consider to remove it. My > idea is to let SkDiscardableMemoryMask inherit SkMask simply to avoid possible > API change, such as filterMask(...), etc. I have updated the CL to avoid subclassing of SkMask. It was applied above SKia trunk 15017 (https://skia.googlecode.com/svn/trunk@15017). @reed and @reveman, please help to take another look.
On 2014/06/05 08:42:42, Jun Jiang wrote: > On 2014/06/04 15:28:09, junj wrote: > > On 2014/06/04 15:14:38, reed1 wrote: > > > tried to apply locally, and failed to merge. can you rebase? > > > > > > Off hand, the virtual on SkMask makes me nervous, as that would stop us from > > > being able to make copies of masks, as we do with bitmaps today. We work > very > > > hard to not allow subclassing of bitmaps (and masks). > > > > @reed, thanks for your quick reply and comments. > > > > It seems to me the CL is already rebased against latest code. The last commit > > below is as following: > > commit a4bb4cf7c401d9f0aed34ab0f26cdce2d1336045 > > Author: mailto:borenet@google.com > > <mailto:borenet@google.com@2bbb7eff-a529-9590-31e7-b0007b416f81> > > Date: Mon Jun 2 11:48:44 2014 +0000 > > > > Whitespace change to verify that git_merger is off > > > > Review URL: https://codereview.chromium.org/312523002 > > > > git-svn-id: https://skia.googlecode.com/svn/trunk@15017 > > 2bbb7eff-a529-9590-31e7-b0007b416f81 > > > > As for the virtual functions added to SkMask, I will consider to remove it. My > > idea is to let SkDiscardableMemoryMask inherit SkMask simply to avoid possible > > API change, such as filterMask(...), etc. > > I have updated the CL to avoid subclassing of SkMask. It was applied above SKia > trunk 15017 (https://skia.googlecode.com/svn/trunk@15017). > > @reed and @reveman, please help to take another look. I found the skia code in svn trunk was older than in that in git. I had rebased the patch against latest code in git. It should be applied to the git now. Please help to take a look. Thanks.
Why do you need a cache besides SkScaledImageCache? https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cpp File src/core/SkMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cp... src/core/SkMaskFilter.cpp:222: SkScaledImageCache::Unlock( Who set fCacheId before this Unlock call?
https://codereview.chromium.org/286273002/diff/60001/include/core/SkMask.h File include/core/SkMask.h (right): https://codereview.chromium.org/286273002/diff/60001/include/core/SkMask.h#ne... include/core/SkMask.h:139: struct SkDiscardableMemoryMask { Naming nit: instead of baking "discardable memory" into the name, maybe this could be something a bit more generic, like SkCachedMask? https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.cpp File src/effects/SkBlurMask.cpp (right): https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.c... src/effects/SkBlurMask.cpp:638: SkScaledImageCache::Unlock(static_cast<SkScaledImageCache::ID*>(dm->fCacheId)); This 5-line stanza is repeated 3 times. Could we refactor this into some kind of guard class that does either scaled image caching or regular allocations, so we don't lose the RAII? Just a thought. https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.h File src/effects/SkBlurMask.h (right): https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.h... src/effects/SkBlurMask.h:81: static SkDiscardableMemoryMask* getDiscardableMemoryMaskFromMap(SkMask*); Naming nit: similarly, could this be getCachedMask()? No need to expose that it's a map or that it's backed by discardable memory, AFAICT. https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... File src/effects/SkBlurMaskFilter.cpp (right): https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:9: #include <list> I'm not sure what the Skia project's official stance is on STL (there's no mention of it in the style guide), but there seems to be a preference to using Skia's home-baked collections. OTOH, I don't know of a Skia-specific list template, so this comment is not really actionable. https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:180: found = false; Perhaps this inner loop could be in its own function? Then the bool assignment here wouldn't be necessary, and an early-return would suffice. https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... src/effects/SkBlurMaskFilter.cpp:187: found = false; Same here.
I am also not in favor of using stl for collections. My high-level question about the need for 2 caches also addresses that: if we can just use scaledimagecache, we may not need these collections.
On 2014/06/09 14:55:34, reed1 wrote: > Why do you need a cache besides SkScaledImageCache? > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cpp > File src/core/SkMaskFilter.cpp (right): > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cp... > src/core/SkMaskFilter.cpp:222: SkScaledImageCache::Unlock( > Who set fCacheId before this Unlock call? Hi, reed. Thanks for your comments. Following is my understanding: 1. SkScaledImageCache can be only used to cache the *blured mask data*. We need to set up an association of *sigma*, *rects* and *blured mask data* for lookup and find matches. Therefore, we need another class BlurMaskRecord to hold and cache this kind of association. That is, SkScaledImageCache is to store the resulted mask data. And BlurMaskRecord is to store the association and pairs for lookup. 2. fCacheId is set when SkScaledImageCache::AddAndLock is called in SkBlurMask::BlurRect or SkBlurMask::BoxBlur. SkScaledImageCache::lock will be called either when the ImageCache is first created or when the ImageCache is fetched from the BlurMaskRecord cache for use. And the SkScaledImageCache::unlock will be called exactly when the NinePatch is going to be destructed. This guarantees that if the ImageCache is not in use, it is in unlocked state; While if it is in use, it is locked and the Image Cache won't be purged.
On 2014/06/10 14:33:12, junj wrote: > On 2014/06/09 14:55:34, reed1 wrote: > > Why do you need a cache besides SkScaledImageCache? > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cpp > > File src/core/SkMaskFilter.cpp (right): > > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cp... > > src/core/SkMaskFilter.cpp:222: SkScaledImageCache::Unlock( > > Who set fCacheId before this Unlock call? > > Hi, reed. Thanks for your comments. Following is my understanding: > 1. SkScaledImageCache can be only used to cache the *blured mask data*. > We need to set up an association of *sigma*, *rects* and *blured mask data* > for lookup and find matches. > Therefore, we need another class BlurMaskRecord to hold and cache this kind > of association. > That is, SkScaledImageCache is to store the resulted mask data. And > BlurMaskRecord is to store the association and pairs for lookup. > 2. fCacheId is set when SkScaledImageCache::AddAndLock is called in > SkBlurMask::BlurRect or SkBlurMask::BoxBlur. > SkScaledImageCache::lock will be called either when the ImageCache is first > created or when the ImageCache is fetched from the BlurMaskRecord cache for use. > And the SkScaledImageCache::unlock will be called exactly when the NinePatch is > going to be destructed. This guarantees that if the ImageCache is not in use, it > is in unlocked state; While if it is in use, it is locked and the Image Cache > won't be purged. Hmmm, I hate to create another cache, with another budget. Perhaps we can generalize the "key" part of scaledimagecache (and rename it) so it can take arbitraryish keys (e.g. sigma+rects).
On 2014/06/09 18:03:58, Stephen White wrote: > https://codereview.chromium.org/286273002/diff/60001/include/core/SkMask.h > File include/core/SkMask.h (right): > > https://codereview.chromium.org/286273002/diff/60001/include/core/SkMask.h#ne... > include/core/SkMask.h:139: struct SkDiscardableMemoryMask { > Naming nit: instead of baking "discardable memory" into the name, maybe this > could be something a bit more generic, like SkCachedMask? > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.cpp > File src/effects/SkBlurMask.cpp (right): > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.c... > src/effects/SkBlurMask.cpp:638: > SkScaledImageCache::Unlock(static_cast<SkScaledImageCache::ID*>(dm->fCacheId)); > This 5-line stanza is repeated 3 times. Could we refactor this into some kind of > guard class that does either scaled image caching or regular allocations, so we > don't lose the RAII? Just a thought. > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.h > File src/effects/SkBlurMask.h (right): > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMask.h... > src/effects/SkBlurMask.h:81: static SkDiscardableMemoryMask* > getDiscardableMemoryMaskFromMap(SkMask*); > Naming nit: similarly, could this be getCachedMask()? No need to expose that > it's a map or that it's backed by discardable memory, AFAICT. > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... > File src/effects/SkBlurMaskFilter.cpp (right): > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:9: #include <list> > I'm not sure what the Skia project's official stance is on STL (there's no > mention of it in the style guide), but there seems to be a preference to using > Skia's home-baked collections. OTOH, I don't know of a Skia-specific list > template, so this comment is not really actionable. > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:180: found = false; > Perhaps this inner loop could be in its own function? Then the bool assignment > here wouldn't be necessary, and an early-return would suffice. > > https://codereview.chromium.org/286273002/diff/60001/src/effects/SkBlurMaskFi... > src/effects/SkBlurMaskFilter.cpp:187: found = false; > Same here. Hi, Stephen. Thanks for your comments and suggestions. I am considering to update the CL to address your comments. As for the issue on using STL list, I understand the concern you and reed had in mind. If we need to avoid using STL, one possible way I can think out is to use a generic list as it did for SkScaledImageCache::Rec in src/core/SkScaledImageCache.cpp.
On 2014/06/10 14:33:12, junj wrote: > On 2014/06/09 14:55:34, reed1 wrote: > > Why do you need a cache besides SkScaledImageCache? > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cpp > > File src/core/SkMaskFilter.cpp (right): > > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cp... > > src/core/SkMaskFilter.cpp:222: SkScaledImageCache::Unlock( > > Who set fCacheId before this Unlock call? > > Hi, reed. Thanks for your comments. Following is my understanding: > 1. SkScaledImageCache can be only used to cache the *blured mask data*. > We need to set up an association of *sigma*, *rects* and *blured mask data* > for lookup and find matches. > Therefore, we need another class BlurMaskRecord to hold and cache this kind > of association. > That is, SkScaledImageCache is to store the resulted mask data. And > BlurMaskRecord is to store the association and pairs for lookup. > 2. fCacheId is set when SkScaledImageCache::AddAndLock is called in > SkBlurMask::BlurRect or SkBlurMask::BoxBlur. > SkScaledImageCache::lock will be called either when the ImageCache is first > created or when the ImageCache is fetched from the BlurMaskRecord cache for use. > And the SkScaledImageCache::unlock will be called exactly when the NinePatch is > going to be destructed. This guarantees that if the ImageCache is not in use, it > is in unlocked state; While if it is in use, it is locked and the Image Cache > won't be purged. What's the limit on the size of this secondary cache? Are entries ever deleted? I couldn't see any calls to clearBlurMaskRecordList() in the patch.
On 2014/06/10 14:35:35, reed1 wrote: > On 2014/06/10 14:33:12, junj wrote: > > On 2014/06/09 14:55:34, reed1 wrote: > > > Why do you need a cache besides SkScaledImageCache? > > > > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cpp > > > File src/core/SkMaskFilter.cpp (right): > > > > > > > > > https://codereview.chromium.org/286273002/diff/60001/src/core/SkMaskFilter.cp... > > > src/core/SkMaskFilter.cpp:222: SkScaledImageCache::Unlock( > > > Who set fCacheId before this Unlock call? > > > > Hi, reed. Thanks for your comments. Following is my understanding: > > 1. SkScaledImageCache can be only used to cache the *blured mask data*. > > We need to set up an association of *sigma*, *rects* and *blured mask > data* > > for lookup and find matches. > > Therefore, we need another class BlurMaskRecord to hold and cache this kind > > of association. > > That is, SkScaledImageCache is to store the resulted mask data. And > > BlurMaskRecord is to store the association and pairs for lookup. > > 2. fCacheId is set when SkScaledImageCache::AddAndLock is called in > > SkBlurMask::BlurRect or SkBlurMask::BoxBlur. > > SkScaledImageCache::lock will be called either when the ImageCache is first > > created or when the ImageCache is fetched from the BlurMaskRecord cache for > use. > > And the SkScaledImageCache::unlock will be called exactly when the NinePatch > is > > going to be destructed. This guarantees that if the ImageCache is not in use, > it > > is in unlocked state; While if it is in use, it is locked and the Image Cache > > won't be purged. > > Hmmm, I hate to create another cache, with another budget. Perhaps we can > generalize the "key" part of scaledimagecache (and rename it) so it can take > arbitraryish keys (e.g. sigma+rects). It is quite insightful to *generalize* the "key" part of scaledimagecache. I will check if I can find a easy way to transform the pairs(sigma + rects) into *meaning keys*, especially when the rects is varying by rect counts, size and relevant positions when rect count is 2.
mtklein is pretty good a thinking about skia's keys and caches...
On 2014/06/10 15:46:47, reed1 wrote:
> mtklein is pretty good a thinking about skia's keys and caches...
When I got time to come back to this issue again, I found that it seems easy to
*generalize* the key but I have no idea on how to produce the hash
key(en.wikipedia.org/wiki/MurmurHash) for a *generalized key*. In fact, I am not
quite sure on why Key::operator<() uses fGenID for comparison while
Key::operator==() uses fHash for comparison. It seems to me it is OK to use
fGenID for both cases.
One of the model of generalized key I thought can be like this:
Key {
unsigned fLength; // length of the valid data in fData
void * fData; // pointer to hold the data
}
It can hold complicated condition for keys. But it may be difficult to generate
Hash key for it.
@mtklein and @reed, any comments or suggestions on this? especially on the
interface changes.
On 2014/06/16 13:24:45, junj wrote:
> On 2014/06/10 15:46:47, reed1 wrote:
> > mtklein is pretty good a thinking about skia's keys and caches...
>
> When I got time to come back to this issue again, I found that it seems easy
to
> *generalize* the key but I have no idea on how to produce the hash
> key(en.wikipedia.org/wiki/MurmurHash) for a *generalized key*. In fact, I am
not
> quite sure on why Key::operator<() uses fGenID for comparison while
> Key::operator==() uses fHash for comparison. It seems to me it is OK to use
> fGenID for both cases.
>
> One of the model of generalized key I thought can be like this:
>
> Key {
> unsigned fLength; // length of the valid data in fData
> void * fData; // pointer to hold the data
> }
>
> It can hold complicated condition for keys. But it may be difficult to
generate
> Hash key for it.
>
> @mtklein and @reed, any comments or suggestions on this? especially on the
> interface changes.
If you'd like to use an arbitrary byte string for a key, you might be able to
use one of the static methods in SkChecksum.h as your hash (SkChecksum::Compute
is about 5x faster than SkChecksum::Murmur3, but of much worse quality).
Those hash functions are currently set up to take 4-byte-aligned strings.
Normally we don't find this extra constraint to be too big a burden, but if you
can't deal with that I don't think we'd mind generalizing those hash functions a
bit.
> In fact, I am not quite sure on why Key::operator<() uses fGenID for comparison while > Key::operator==() uses fHash for comparison. It seems to me it is OK to use fGenID for > both cases. I think what you're seeing in operator< and operator== is some low-level byte punning. In operator<, we're reading 28 bytes starting from &fGenID: this covers fGenID, fScaleX, fScaleY, and fBounds. So it's, compare all the data, except skip the hash. In operator==, we're comparing _all_ the data, including the hash. Presumably the hash is there only as an accelerator for operator==? Putting it first means a hash mismatch lets us short-circuit the rest of operator==. Arguably, these functions should be written out to compare each of the fields in turn explictly, or should use a function like memcmp if we're going to stick with the byte thinking.
On 2014/06/16 14:25:01, mtklein wrote: > > In fact, I am not quite sure on why Key::operator<() uses fGenID for > comparison while > > Key::operator==() uses fHash for comparison. It seems to me it is OK to use > fGenID for > > both cases. > > I think what you're seeing in operator< and operator== is some low-level byte > punning. In operator<, we're reading 28 bytes starting from &fGenID: this > covers fGenID, fScaleX, fScaleY, and fBounds. So it's, compare all the data, > except skip the hash. In operator==, we're comparing _all_ the data, including > the hash. Presumably the hash is there only as an accelerator for operator==? > Putting it first means a hash mismatch lets us short-circuit the rest of > operator==. Arguably, these functions should be written out to compare each of > the fields in turn explictly, or should use a function like memcmp if we're > going to stick with the byte thinking. Hi, mtklein. Thanks for your comments. Sorry for my late reply since I was busy dealing with some urgent stuff in this whole month. To move this issue forward, my colleague QianKun(qiankun.miao@intel.com) will continue to follow this and move it forward. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
