Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(27)

Issue 258183002: Don't bother doing the empty clip check in SkRecordDraw. (Closed)

Created:
6 years, 7 months ago by mtklein_C
Modified:
6 years, 7 months ago
CC:
skia-review_googlegroups.com
Base URL:
https://skia.googlesource.com/skia.git@master
Visibility:
Public.

Description

Don't bother doing the empty clip check in SkRecordDraw. On Mike's suggestion, I tested out not doing any empty-clip check at all in SkRecordDraw, given that mostly we'll do that again anyway inside SkCanvas. Most SKPs are identical to the status quo, whether bot or silk, played back in tiles or full. Average playback performance, both arithmetic and geometric mean, is also unchanged. A handful of SKPs do draw faster or slower reliably, particularly when tiled. E.g. a cnn tile draws about 40% faster, a cuteoverload tile about 20% slower. Their profiles look pretty much the same before and after, so I can't really explain the changes. I'd say, given that performance is mostly identical and very identical in bulk, we might as well remove this code. It's nice to keep SkRecordDraw as dumb as possible. BUG=skia:2378 Committed: http://code.google.com/p/skia/source/detail?r=14433

Patch Set 1 #

Patch Set 2 : comments and tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -38 lines) Patch
M src/record/SkRecordDraw.cpp View 1 2 chunks +7 lines, -14 lines 0 comments Download
M tests/RecordDrawTest.cpp View 1 1 chunk +0 lines, -24 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
mtklein
6 years, 7 months ago (2014-04-28 22:35:50 UTC) #1
reed1
lgtm
6 years, 7 months ago (2014-04-29 12:29:57 UTC) #2
f(malita)
lgtm
6 years, 7 months ago (2014-04-29 12:42:09 UTC) #3
mtklein
The CQ bit was checked by mtklein@google.com
6 years, 7 months ago (2014-04-29 13:01:40 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/258183002/20001
6 years, 7 months ago (2014-04-29 13:01:46 UTC) #5
mtklein
The CQ bit was unchecked by mtklein@google.com
6 years, 7 months ago (2014-04-29 13:52:01 UTC) #6
mtklein
The CQ bit was checked by mtklein@google.com
6 years, 7 months ago (2014-04-29 13:52:09 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/258183002/20001
6 years, 7 months ago (2014-04-29 13:52:26 UTC) #8
borenet
The CQ bit was unchecked by borenet@google.com
6 years, 7 months ago (2014-04-29 15:27:07 UTC) #9
borenet
The CQ bit was checked by borenet@google.com
6 years, 7 months ago (2014-04-29 15:27:13 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/258183002/20001
6 years, 7 months ago (2014-04-29 15:27:24 UTC) #11
commit-bot: I haz the power
6 years, 7 months ago (2014-04-29 15:34:08 UTC) #12
Message was sent while issue was closed.
Change committed as 14433

Powered by Google App Engine
This is Rietveld 408576698