cc: Optimize ImageDecodeCacheKey::FromDrawImage.

cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest. This is a frequently used function
when it comes to dealing with software images, so any savings should
be good.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s (+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2669933002
Cr-Commit-Position: refs/heads/master@{#447882}
Committed: https://chromium.googlesource.com/chromium/src/+/13778fbf14dab7f317d3679e75dbdd1aad28697e

[vmpstr, 2017-02-02 00:09:37]

Description was changed from

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
==========

to

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-02-02 00:10:59]

Description was changed from

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest. This is a frequently used function
when it comes to dealing with software images, so any savings should
be good.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-02-02 00:11:31]

Please take a look.

[enne (OOO), 2017-02-02 00:36:42]

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:182: gfx::Rect GetSrcRect(const
DrawImage& image) {
Does this end up being cheaper than the code that it replaces? It's just saving
the work of constructing objects that don't get inlined?

The other bit where you don't bother looking at rects for small images is
understandable to me why it would make things faster, but was curious how much
this helped.

[ericrk, 2017-02-02 00:37:56]

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:182: gfx::Rect GetSrcRect(const
DrawImage& image) {
Can we just store this on DrawImage rather than src_rect? seems like the only
uses of src_rect are to compute this.

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:183: const SkIRect& src_rect =
image.src_rect();
DrawImage::src_rect() returns a "const SkIRect", not a "const SkIRect&", so this
will cause a copy - can we update it to return const&? may make this even faster
:D

[vmpstr, 2017-02-02 00:52:45]

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:182: gfx::Rect GetSrcRect(const
DrawImage& image) {
On 2017/02/02 00:36:41, enne wrote:
> Does this end up being cheaper than the code that it replaces? It's just
saving
> the work of constructing objects that don't get inlined?
> 
> The other bit where you don't bother looking at rects for small images is
> understandable to me why it would make things faster, but was curious how much
> this helped.

This alone speeds up the test by about 25%. There are two things here:
1. We skip the "IsEmpty" checks, which works because we know we don't get empty
images even if we do, we know they are at least at (0, 0) (they don't go
negative), so the check on 188 covers the empty checks.
2. We skip gfx::SkIRectToRect, which showed up surprisingly hot (10% of the
test)

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:182: gfx::Rect GetSrcRect(const
DrawImage& image) {
On 2017/02/02 00:37:56, ericrk wrote:
> Can we just store this on DrawImage rather than src_rect? seems like the only
> uses of src_rect are to compute this.

Hmm, I'm a little hesitant to do this, since there are other call sites (like
mip calculation and gpu cache). It's not clear for example that returning empty
when rects don't intersect would be a reasonable thing. src_rect as far as i've
been using it on draw image is more of an input thing.

I can try it out if you think it's better. WDYT?

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:183: const SkIRect& src_rect =
image.src_rect();
On 2017/02/02 00:37:56, ericrk wrote:
> DrawImage::src_rect() returns a "const SkIRect", not a "const SkIRect&", so
this
> will cause a copy - can we update it to return const&? may make this even
faster
> :D

Done, the perf results are comparable.

[ericrk, 2017-02-02 22:21:28]

lgtm

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:182: gfx::Rect GetSrcRect(const
DrawImage& image) {
On 2017/02/02 00:52:45, vmpstr wrote:
> On 2017/02/02 00:37:56, ericrk wrote:
> > Can we just store this on DrawImage rather than src_rect? seems like the
only
> > uses of src_rect are to compute this.
> 
> Hmm, I'm a little hesitant to do this, since there are other call sites (like
> mip calculation and gpu cache). It's not clear for example that returning
empty
> when rects don't intersect would be a reasonable thing. src_rect as far as
i've
> been using it on draw image is more of an input thing.
> 
> I can try it out if you think it's better. WDYT?

I see, the full computations aren't needed everywhere.. keeping it as-is sgtm.

https://codereview.chromium.org/2669933002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:183: const SkIRect& src_rect =
image.src_rect();
On 2017/02/02 00:52:45, vmpstr wrote:
> On 2017/02/02 00:37:56, ericrk wrote:
> > DrawImage::src_rect() returns a "const SkIRect", not a "const SkIRect&", so
> this
> > will cause a copy - can we update it to return const&? may make this even
> faster
> > :D
> 
> Done, the perf results are comparable. 

Done.

[enne (OOO), 2017-02-02 22:22:53]

lgtm

[vmpstr, 2017-02-02 22:29:46]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-02-02 22:30:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 23:44:05]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486074586842710,
"parent_rev": "ff3e5ef957ccb585c7d73a0b011bdd432f5e57e0", "commit_rev":
"13778fbf14dab7f317d3679e75dbdd1aad28697e"}

[commit-bot: I haz the power, 2017-02-02 23:44:34]

Description was changed from

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest. This is a frequently used function
when it comes to dealing with software images, so any savings should
be good.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Optimize ImageDecodeCacheKey::FromDrawImage.

This patch reduces the time spent in ::FromDrawImage call by 50%. Comes
with a corresponding perftest. This is a frequently used function
when it comes to dealing with software images, so any savings should
be good.

Perftest result without the sidc change (z620):
*RESULT software_image_decode_cache_fromdrawimage: result= 318435.53125 runs/s

With the change:
*RESULT software_image_decode_cache_fromdrawimage: result= 657221.0625 runs/s
(+106%)

R=enne@chromium.org, ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2669933002
Cr-Commit-Position: refs/heads/master@{#447882}
Committed:
https://chromium.googlesource.com/chromium/src/+/13778fbf14dab7f317d3679e75db...
==========

[commit-bot: I haz the power, 2017-02-02 23:44:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/13778fbf14dab7f317d3679e75db...