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

Issue 793033002: SkRecord: increase min block to 512B, remove max. (Closed)

Created:
6 years ago by mtklein_C
Modified:
6 years ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Project:
skia
Visibility:
Public.

Description

SkRecord: increase min block to 512B, remove max. When we added the 64K allocation cap, the bots showed we took a perf hit on some large .skps like desk_pokemonwiki.skp, despite not seeing a local effect. I'm still not seeing that locally, but I'd like to try removing the cap on the bots to see what happens. For big monolithic pictures, really packing into memory tightly is probably not as important as it is for tiny ones. Similarly, we're probably being too cautious about making tiny allocations. Today we start at 16 bytes, which isn't really enough to record anything. Even the smallest picture, say, save clipRect drawRect restore requires ~200 bytes, so we might as well move our minimum block size up near there. I don't know if 16 bytes is too small to start for GrTextStrikes, so I've left the behavior the same (though the max is still gone). Local recording performance is neutral-to-positive: tabl_deviantart.skp 126us -> 129us 1.02x tabl_nytimes.skp 110us -> 112us 1.02x tabl_cuteoverload.skp 521us -> 530us 1.02x desk_mobilenews.skp 673us -> 682us 1.01x desk_chalkboard.skp 843us -> 854us 1.01x desk_sfgate.skp 528us -> 535us 1.01x desk_silkfinance.skp 68.2us -> 69us 1.01x desk_youtube.skp 623us -> 629us 1.01x desk_blogger.skp 472us -> 475us 1.01x desk_jsfiddlehumperclip.skp 42.2us -> 42.5us 1.01x desk_espn.skp 255us -> 256us 1.01x desk_ebay.skp 174us -> 174us 1x desk_twitter.skp 454us -> 455us 1x tabl_pravda.skp 200us -> 201us 1x desk_wordpress.skp 782us -> 784us 1x desk_samoasvg.skp 762us -> 761us 1x tabl_mozilla.skp 1.58ms -> 1.58ms 1x tabl_slashdot.skp 107us -> 107us 1x tabl_techmeme.skp 102us -> 102us 0.99x tabl_gamedeksiam.skp 729us -> 724us 0.99x tabl_nofolo.skp 65.3us -> 64.7us 0.99x desk_gmailthread.skp 339us -> 336us 0.99x tabl_sahadan.skp 91us -> 90us 0.99x desk_yahooanswers.skp 144us -> 142us 0.99x tabl_cnet.skp 143us -> 141us 0.99x tabl_googleblog.skp 206us -> 203us 0.99x tabl_cnn.skp 160us -> 158us 0.99x tabl_frantzen.skp 50.5us -> 49.6us 0.98x desk_linkedin.skp 328us -> 323us 0.98x tabl_digg.skp 790us -> 769us 0.97x desk_jsfiddlebigcar.skp 40.6us -> 39.5us 0.97x desk_mapsvg.skp 1.57ms -> 1.52ms 0.97x tabl_gmail.skp 19.4us -> 18.6us 0.96x tabl_hsfi.skp 9.81us -> 9.11us 0.93x BUG=skia: Committed: https://skia.googlesource.com/skia/+/59dba146fe4170c4986b2494414c08be2c93d4a2

Patch Set 1 #

Patch Set 2 : 8 #

Patch Set 3 : Leave GrTextStrike alone. #

Patch Set 4 : order #

Patch Set 5 : explicit, test #

Patch Set 6 : bump Gr #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -17 lines) Patch
M src/core/SkRecord.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkVarAlloc.h View 1 2 3 4 2 chunks +3 lines, -2 lines 0 comments Download
M src/core/SkVarAlloc.cpp View 1 2 4 chunks +3 lines, -12 lines 0 comments Download
M src/gpu/GrFontCache.cpp View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M tests/VarAllocTest.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 13 (4 generated)
mtklein_C
6 years ago (2014-12-10 21:49:58 UTC) #2
joshualitt
On 2014/12/10 21:49:58, mtklein_C wrote: Mike, 512 bytes works great for TextStrike purposes. Even a ...
6 years ago (2014-12-10 22:00:35 UTC) #3
mtklein
On 2014/12/10 22:00:35, joshualitt wrote: > On 2014/12/10 21:49:58, mtklein_C wrote: > > Mike, 512 ...
6 years ago (2014-12-10 22:17:57 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/793033002/100001
6 years ago (2014-12-10 22:18:52 UTC) #6
commit-bot: I haz the power
Note for Reviewers: The CQ is waiting for an approval. If you believe that the ...
6 years ago (2014-12-10 22:18:53 UTC) #7
commit-bot: I haz the power
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full ...
6 years ago (2014-12-11 04:18:55 UTC) #9
joshualitt
On 2014/12/11 04:18:55, I haz the power (commit-bot) wrote: > No LGTM from a valid ...
6 years ago (2014-12-11 14:22:00 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/793033002/100001
6 years ago (2014-12-12 15:32:10 UTC) #12
commit-bot: I haz the power
6 years ago (2014-12-12 16:41:35 UTC) #13
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/59dba146fe4170c4986b2494414c08be2c93d4a2

Powered by Google App Engine
This is Rietveld 408576698