Cache blur mask for rects which can not break into nine-patch

Cache blur mask for rects which can not break into nine-patch

With this CL performance improves:
blurrectsnonninepatch   42.4us -> 20.5us        0.48x

BUG=431021, skia:3118
Committed: https://skia.googlesource.com/skia/+/5a5b4e900ee69540fc35952882a323c63d6d0db9

[qiankun, 2014-11-14 09:42:09]

qiankun.miao@intel.com changed reviewers:
+ reed@google.com

[qiankun, 2014-11-14 09:42:10]

reed, I made a prototype to cache result mask for rects which can not break into
nine-patch. I just copied find/add from SkBlurMaskFilter.cpp now. Maybe I can
put these common functions in SkMaskCache.cpp. PTAL.

[reed1, 2014-11-14 14:29:14]

reed@google.com changed reviewers:
+ humper@google.com

[qiankun, 2014-11-19 16:14:58]

PTAL.

[qiankun, 2014-11-25 15:30:58]

Ping. Any comments?

[reed1, 2014-12-05 19:22:17]

Sorry for the long absence. Things were very busy here the last few weeks.

Can we construct a GM/bench that exercises these cases?

[qiankun, 2014-12-09 08:02:20]

On 2014/12/05 19:22:17, reed1 wrote:
> Sorry for the long absence. Things were very busy here the last few weeks.
> 
> Can we construct a GM/bench that exercises these cases?

Thanks for review. I added GM/bench. bench data as follows:
before:
10M   5       43.3µs  43.4µs  43.6µs  44.5µs  1%      ▆▂▂▂▂█▂▂▁▁      8888   
blurrectsnonninepatch
after:
10M   4       20.6µs  20.8µs  21.1µs  23.5µs  4%      █▃▂▁▁▁▁▁▃▂      8888   
blurrectsnonninepatch

[reed1, 2014-12-09 16:42:33]

if possible, landing bench/BlurRectsBench.cpp as its own CL now would be fine
(and help confirm the perf difference after the main CL lands).

[reed1, 2014-12-09 18:28:50]

actually, I see that we also want the new GM to land ahead of time.

[reed1, 2014-12-09 18:46:34]

On 2014/12/09 18:28:50, reed1 wrote:
> actually, I see that we also want the new GM to land ahead of time.

when we add it, lets offset the x or y in the 2nd draw by some fractional amount
(e.g. 0.1) as well, so we can test the cache for fractional phases.

[reed1, 2014-12-09 18:47:08]

For this CL, it shows edge artifacts when I slide the GM around inside SampleApp
(try dragging it to the left).

[qiankun, 2014-12-10 10:44:20]

On 2014/12/09 18:47:08, reed1 wrote:
> For this CL, it shows edge artifacts when I slide the GM around inside
SampleApp
> (try dragging it to the left).

Thanks for catching this. I added a gm to check this.

[qiankun, 2014-12-19 02:23:07]

hi reed, please take a look at this when you free. Thanks.

[reed1, 2014-12-22 21:38:16]

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h
File src/core/SkMaskCache.h (right):

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h#...
src/core/SkMaskCache.h:42: 
Why do these take their parameters in a slightly different order from FindAndRef
/ Add?

Can we name them some way to reflect how they are different from FindAndRef /
Add?
e.g. FindAndCopy?

Either way, I think we need some dox to describe what these do, esp. how they
are different.

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:269: || !rectCount
This 4-part predicate may be correct, but it is pretty complex for me to read.
Can it be restructured to make it clearer?

[qiankun, 2014-12-23 10:54:33]

Uploaded a new patch to fix the comments. PTAL.

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h
File src/core/SkMaskCache.h (right):

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskCache.h#...
src/core/SkMaskCache.h:42: 
On 2014/12/22 21:38:15, reed1 wrote:
> Why do these take their parameters in a slightly different order from
FindAndRef
> / Add?
> 
> Can we name them some way to reflect how they are different from FindAndRef /
> Add?
> e.g. FindAndCopy?
> 
> Either way, I think we need some dox to describe what these do, esp. how they
> are different.
Modify the name and re-order parameters for FindAndCopy/AddAndCopy API. Add some
comments to point the difference with FindAndRef/Add.

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/100001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:269: || !rectCount
On 2014/12/22 21:38:16, reed1 wrote:
> This 4-part predicate may be correct, but it is pretty complex for me to read.
> Can it be restructured to make it clearer?

What about the latest implementation?

[qiankun, 2015-01-06 05:40:15]

Patchset #9 (id:160001) has been deleted

[qiankun, 2015-01-07 14:22:40]

Ping.

[reed1, 2015-01-07 16:45:47]

much clearer, thanks.

lgtm w/ nits

https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:269: if
(!SkMaskCache::FindAndCopy(matrix.mapRadius(rec.fSigma), rec.fStyle,
nit: tmp variable caching the computed radius?

SkScalar scaledSigma = mapRadius(rec.fSigma);

https://codereview.chromium.org/729463002/diff/180001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/180001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:502: if (!SkMaskCache::FindAndCopy(sigma,
fBlurStyle, this->getQuality(), smallR, count, &patch->fMask)) {
nit: 100col max

[qiankun, 2015-01-08 02:27:45]

The CQ bit was checked by qiankun.miao@intel.com

[commit-bot: I haz the power, 2015-01-08 02:28:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/729463002/200001

[commit-bot: I haz the power, 2015-01-08 02:37:50]

Committed patchset #10 (id:200001) as
https://skia.googlesource.com/skia/+/5a5b4e900ee69540fc35952882a323c63d6d0db9

[qiankun, 2015-01-08 03:01:12]

Thank you reed!

https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/180001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:269: if
(!SkMaskCache::FindAndCopy(matrix.mapRadius(rec.fSigma), rec.fStyle,
On 2015/01/07 16:45:46, reed1 wrote:
> nit: tmp variable caching the computed radius?
> 
> SkScalar scaledSigma = mapRadius(rec.fSigma);

Done.

https://codereview.chromium.org/729463002/diff/180001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/180001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:502: if (!SkMaskCache::FindAndCopy(sigma,
fBlurStyle, this->getQuality(), smallR, count, &patch->fMask)) {
On 2015/01/07 16:45:46, reed1 wrote:
> nit: 100col max

Done.

[reed2, 2015-01-08 03:01:13]

reed@chromium.org changed reviewers:
+ reed@chromium.org

[reed2, 2015-01-08 03:01:14]

Looks like this is leaking memory...

==8017==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 25992 byte(s) in 2 object(s) allocated from:
    #0 0x7feb53030e0b in __interceptor_malloc
(/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/gm+0x24fe0b)
    #1 0x7feb54d54f76 in sk_malloc_flags(unsigned long, unsigned int)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:54:15
    #2 0x7feb54d54d4a in sk_malloc_throw(unsigned long)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:40:12
    #3 0x7feb539f5a77 in SkMask::AllocImage(unsigned long)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMask.cpp:37:22
    #4 0x7feb53fe5c34 in (anonymous
namespace)::copy_cacheddata_to_mask(SkCachedData*, SkMask*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:25:20
    #5 0x7feb53fea064 in SkMaskCache::FindAndCopy(float, SkBlurStyle,
SkBlurQuality, SkRect const*, int, SkMask*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:224:26
    #6 0x7feb539f957e in SkMaskFilter::filterPath(SkPath const&, SkMatrix
const&, SkRasterClip const&, SkBlitter*, SkPaint::Style) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskFilter.cpp:270:14
    #7 0x7feb5392e920 in SkDraw::drawPath(SkPath const&, SkPaint const&,
SkMatrix const*, bool, bool, SkBlitter*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkDraw.cpp:1117:13
    #8 0x7feb53694afc in SkDraw::drawPath(SkPath const&, SkPaint const&,
SkMatrix const*, bool) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../include/core/SkDraw.h:54:9
    #9 0x7feb5368d799 in SkBitmapDevice::drawPath(SkDraw const&, SkPath const&,
SkPaint const&, SkMatrix const*, bool)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkBitmapDevice.cpp:216:5
    #10 0x7feb5386aa57 in SkCanvas::onDrawPath(SkPath const&, SkPaint const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1908:9
    #11 0x7feb5386386b in SkCanvas::drawPath(SkPath const&, SkPaint const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1703:5
    #12 0x7feb53109572 in Blur2RectsNonNinePatchGM::onDraw(SkCanvas*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../gm/blurs.cpp:166:9

[qiankun, 2015-01-08 03:06:19]

On 2015/01/08 03:01:14, reed2 wrote:
> Looks like this is leaking memory...
> 
> ==8017==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 25992 byte(s) in 2 object(s) allocated from:
>     #0 0x7feb53030e0b in __interceptor_malloc
>
(/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/gm+0x24fe0b)
>     #1 0x7feb54d54f76 in sk_malloc_flags(unsigned long, unsigned int)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:54:15
>     #2 0x7feb54d54d4a in sk_malloc_throw(unsigned long)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:40:12
>     #3 0x7feb539f5a77 in SkMask::AllocImage(unsigned long)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMask.cpp:37:22
>     #4 0x7feb53fe5c34 in (anonymous
> namespace)::copy_cacheddata_to_mask(SkCachedData*, SkMask*)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:25:20
>     #5 0x7feb53fea064 in SkMaskCache::FindAndCopy(float, SkBlurStyle,
> SkBlurQuality, SkRect const*, int, SkMask*)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:224:26
>     #6 0x7feb539f957e in SkMaskFilter::filterPath(SkPath const&, SkMatrix
> const&, SkRasterClip const&, SkBlitter*, SkPaint::Style) const
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskFilter.cpp:270:14
>     #7 0x7feb5392e920 in SkDraw::drawPath(SkPath const&, SkPaint const&,
> SkMatrix const*, bool, bool, SkBlitter*) const
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkDraw.cpp:1117:13
>     #8 0x7feb53694afc in SkDraw::drawPath(SkPath const&, SkPaint const&,
> SkMatrix const*, bool) const
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../include/core/SkDraw.h:54:9
>     #9 0x7feb5368d799 in SkBitmapDevice::drawPath(SkDraw const&, SkPath
const&,
> SkPaint const&, SkMatrix const*, bool)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkBitmapDevice.cpp:216:5
>     #10 0x7feb5386aa57 in SkCanvas::onDrawPath(SkPath const&, SkPaint const&)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1908:9
>     #11 0x7feb5386386b in SkCanvas::drawPath(SkPath const&, SkPaint const&)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1703:5
>     #12 0x7feb53109572 in Blur2RectsNonNinePatchGM::onDraw(SkCanvas*)
>
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../gm/blurs.cpp:166:9

I am taking a look it.

[reed2, 2015-01-08 03:11:01]

It is late. I suggest we revert for now, and can review possible fixes tomorrow.

[qiankun, 2015-01-08 03:13:49]

A revert of this CL (patchset #10 id:200001) has been created in
https://codereview.chromium.org/844673002/ by qiankun.miao@intel.com.

The reason for reverting is: revert it due to a memory leak.
==8017==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 25992 byte(s) in 2 object(s) allocated from:
    #0 0x7feb53030e0b in __interceptor_malloc
(/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/gm+0x24fe0b)
    #1 0x7feb54d54f76 in sk_malloc_flags(unsigned long, unsigned int)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:54:15
    #2 0x7feb54d54d4a in sk_malloc_throw(unsigned long)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/ports/SkMemory_malloc.cpp:40:12
    #3 0x7feb539f5a77 in SkMask::AllocImage(unsigned long)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMask.cpp:37:22
    #4 0x7feb53fe5c34 in (anonymous
namespace)::copy_cacheddata_to_mask(SkCachedData*, SkMask*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:25:20
    #5 0x7feb53fea064 in SkMaskCache::FindAndCopy(float, SkBlurStyle,
SkBlurQuality, SkRect const*, int, SkMask*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskCache.cpp:224:26
    #6 0x7feb539f957e in SkMaskFilter::filterPath(SkPath const&, SkMatrix
const&, SkRasterClip const&, SkBlitter*, SkPaint::Style) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkMaskFilter.cpp:270:14
    #7 0x7feb5392e920 in SkDraw::drawPath(SkPath const&, SkPaint const&,
SkMatrix const*, bool, bool, SkBlitter*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkDraw.cpp:1117:13
    #8 0x7feb53694afc in SkDraw::drawPath(SkPath const&, SkPaint const&,
SkMatrix const*, bool) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../include/core/SkDraw.h:54:9
    #9 0x7feb5368d799 in SkBitmapDevice::drawPath(SkDraw const&, SkPath const&,
SkPaint const&, SkMatrix const*, bool)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkBitmapDevice.cpp:216:5
    #10 0x7feb5386aa57 in SkCanvas::onDrawPath(SkPath const&, SkPaint const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1908:9
    #11 0x7feb5386386b in SkCanvas::drawPath(SkPath const&, SkPaint const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../src/core/SkCanvas.cpp:1703:5
    #12 0x7feb53109572 in Blur2RectsNonNinePatchGM::onDraw(SkCanvas*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-xsan-000/build/slave/Test-Ubuntu13_10-GCE-NoGPU-x86_64-Debug-ASAN/build/skia/out/Debug/../../gm/blurs.cpp:166:9.

[qiankun, 2015-01-08 04:25:41]

Fixed the memory leak. Please help to review.

[reed1, 2015-01-08 15:51:21]

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:270: if (!SkMaskCache::FindAndCopy(scaledSigma,
rec.fStyle, rec.fQuality,
I think it will be easier to read if we change this from 

if (!...

to

if (...

so we can see the code that handles a cache-hit right below the FindAndCopy

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:285: // Fallback to original calculation if size of
bounds is different with
I'm afraid I still don't understand the scenario this is catching. We have a
cache "hit", but we can't use it, so we have to delete the copied answer?

[reed1, 2015-01-08 16:11:34]

If this bounds check is related to clipping, perhaps we need to some how
incorporate the clip bounds/size into the key, rather than creating
(potentially) a cache entry that is clipped, which we keep "finding" but can't
reuse. Alternatively perhaps we only cache unclipped results.

[qiankun, 2015-01-12 04:36:26]

Thanks for review.

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.cpp
File src/core/SkMaskFilter.cpp (right):

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:270: if (!SkMaskCache::FindAndCopy(scaledSigma,
rec.fStyle, rec.fQuality,
On 2015/01/08 15:51:21, reed1 wrote:
> I think it will be easier to read if we change this from 
> 
> if (!...
> 
> to
> 
> if (...
> 
> so we can see the code that handles a cache-hit right below the FindAndCopy

Done.

https://codereview.chromium.org/729463002/diff/220001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:285: // Fallback to original calculation if size of
bounds is different with
On 2015/01/08 15:51:21, reed1 wrote:
> I'm afraid I still don't understand the scenario this is catching. We have a
> cache "hit", but we can't use it, so we have to delete the copied answer?

It was used to handle clipping bounds. Now, the new CL don't search the cache
for clipped dst bounds.

[reed1, 2015-01-12 17:44:55]

Does this mean if the blur is clipped (i.e. large or we are tiled) we won't take
advantage of caching?

[qiankun, 2015-01-13 08:04:33]

On 2015/01/12 17:44:55, reed1 wrote:
> Does this mean if the blur is clipped (i.e. large or we are tiled) we won't
take
> advantage of caching?

Yes, if the blur is clipped, cache isn't triggered. I add some code to handle
clipped path. The code is a bit complex now. Could you give some comments?

[qiankun, 2015-01-19 10:55:11]

Ping reed@

[qiankun, 2015-01-23 15:53:54]

Hi reed, please help to review this when you are free, thanks!

[reed1, 2015-01-23 21:18:39]

hmm, I need to think about what our "policy" should be for when to cache or
not-cache masks that are only partly needed (i.e. they are somewhat outside of
the clip).

[qiankun, 2015-01-26 08:12:28]

On 2015/01/23 21:18:39, reed1 wrote:
> hmm, I need to think about what our "policy" should be for when to cache or
> not-cache masks that are only partly needed (i.e. they are somewhat outside of
> the clip).

For the tile case, cached mask could be used by different tiles, though only
part of the whole cached mask is used by each tile. Also we can re-use the
cached mask when scrolling. 
For other clipping cases, if the clipped mask is re-drawn in a short time (e.g.
scrolling), we can also benefit from caching. 
However, we should balance the performance and memory consumption/code
complexity. When you make the decision for the policy, please let me know.
Thanks for review!

[qiankun, 2015-02-10 14:10:42]

Hi reed, do you make decision about the cache policy? BTW, I noted mask which
can break down into nine-patch have same cache behavior, i.e. cache whole mask
for each tile.

[reed1, 2015-02-10 14:47:46]

On 2015/02/10 14:10:42, qiankun wrote:
> Hi reed, do you make decision about the cache policy? BTW, I noted mask which
> can break down into nine-patch have same cache behavior, i.e. cache whole mask
> for each tile.

At the moment I want to table this change, and only cache "reusable" blurs :
e.g. ninepatches.

[qiankun, 2015-02-10 14:54:56]

On 2015/02/10 14:47:46, reed1 wrote:
> On 2015/02/10 14:10:42, qiankun wrote:
> > Hi reed, do you make decision about the cache policy? BTW, I noted mask
which
> > can break down into nine-patch have same cache behavior, i.e. cache whole
mask
> > for each tile.
> 
> At the moment I want to table this change, and only cache "reusable" blurs :
> e.g. ninepatches.

I see. Thanks for reply. I will keep an eye on the cache size in future. Hope we
can find a better method to balance memory and performance.

[reed1, 2015-02-10 14:58:58]

On 2015/02/10 14:54:56, qiankun wrote:
> On 2015/02/10 14:47:46, reed1 wrote:
> > On 2015/02/10 14:10:42, qiankun wrote:
> > > Hi reed, do you make decision about the cache policy? BTW, I noted mask
> which
> > > can break down into nine-patch have same cache behavior, i.e. cache whole
> mask
> > > for each tile.
> > 
> > At the moment I want to table this change, and only cache "reusable" blurs :
> > e.g. ninepatches.
> 
> I see. Thanks for reply. I will keep an eye on the cache size in future. Hope
we
> can find a better method to balance memory and performance.

Yea, this is a hard problem. If we can analyze the Verge's geometry and find
more clever ways to create reusable masks, that would be great.