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

Issue 721313002: Deparameterize SkVarAlloc. (Closed)

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

Description

Deparameterize SkVarAlloc. SkRecord performance is not sensitive to these values, so we can cut some storage. This rearranges the space-remaining logic to use a count of bytes left rather than a pointer to the end, cutting memory usage a little more. An SkVarAlloc used to weigh 20 or 32 bytes which now becomes 16 or 24. I think if I think about it a bit more I can trim off that Block* too, getting us to 12 or 16 bytes. Because we now just always grow by doubling, this CL switches from storing fSmallest to its log base 2. This has the nice effect of never having to worry about it overflowing, and means we can probably squeeze it down into a byte if we want, even 6 bits. BUG=skia: Committed: https://skia.googlesource.com/skia/+/bc415389855888af5a1282ca4b6bee30afa3d69d CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu12-ShuttleA-GTX660-x86-Debug-Trybot Committed: https://skia.googlesource.com/skia/+/f2950b1c4538fa638a594af6beacd3d1434eb74d

Patch Set 1 #

Patch Set 2 : might as well handle overflow sensibly #

Patch Set 3 : that's not right, undo #

Patch Set 4 : store lgN #

Patch Set 5 : try-zero #

Patch Set 6 : remaining #

Patch Set 7 : fixes: update unit test, remove unused header, start at 16 bytes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -29 lines) Patch
M src/core/SkRecord.h View 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkVarAlloc.h View 1 2 3 4 5 2 chunks +6 lines, -9 lines 0 comments Download
M src/core/SkVarAlloc.cpp View 1 2 3 4 5 6 3 chunks +5 lines, -8 lines 0 comments Download
M tests/RecordTest.cpp View 1 2 3 4 5 6 1 chunk +5 lines, -11 lines 0 comments Download

Messages

Total messages: 11 (3 generated)
mtklein
6 years, 1 month ago (2014-11-13 18:29:33 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/721313002/100001
6 years, 1 month ago (2014-11-13 18:30:11 UTC) #4
commit-bot: I haz the power
Note for Reviewers: The CQ is waiting for an approval. If you believe that the ...
6 years, 1 month ago (2014-11-13 18:30:12 UTC) #5
reed1
lgtm
6 years, 1 month ago (2014-11-13 18:50:29 UTC) #6
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/bc415389855888af5a1282ca4b6bee30afa3d69d
6 years, 1 month ago (2014-11-13 18:51:44 UTC) #7
mtklein
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/718203006/ by mtklein@google.com. ...
6 years, 1 month ago (2014-11-13 19:22:30 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/721313002/120001
6 years, 1 month ago (2014-11-13 19:59:00 UTC) #10
commit-bot: I haz the power
6 years, 1 month ago (2014-11-13 20:41:21 UTC) #11
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/f2950b1c4538fa638a594af6beacd3d1434eb74d

Powered by Google App Engine
This is Rietveld 408576698