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

Issue 1081433002: Fix minor undercounting in SkRecord::bytesUsed(). (Closed)

Created:
5 years, 8 months ago by mtklein_C
Modified:
5 years, 8 months ago
Reviewers:
mtklein, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Fix minor undercounting in SkRecord::bytesUsed(). When an SkRecord has more than kInlineRecords ops in it (today, 5 or more), the current logic undercounts the bytes used by the SkRecord by sizeof(Record) * kInlineRecords, i.e. 32 bytes. This isn't a huge deal... by the time you've recorded 5 ops, we're typically up around 1KB anyway, and it's only ever off by that constant 32 bytes, so somewhere between 3% to 0% error as the picture grows. But now that I've noticed, we might as well fix it. Basically, this is a reminder that the inline space used to store those first kInlineRecords ops goes to waste once we pass that threshold. In contrast, fInlineAlloc, the space we preallocate for the SkVarAlloc, never goes to waste. It always holds the first few ops' data even when we grow past it. BUG=skia: Committed: https://skia.googlesource.com/skia/+/1dda2194e0c3a486032b2dd4d3eaea67fa47d818

Patch Set 1 #

Patch Set 2 : add a comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -3 lines) Patch
M src/core/SkRecord.cpp View 1 1 chunk +7 lines, -3 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
mtklein_C
5 years, 8 months ago (2015-04-10 18:02:08 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081433002/1
5 years, 8 months ago (2015-04-10 18:02:27 UTC) #4
commit-bot: I haz the power
This issue passed the CQ dry run.
5 years, 8 months ago (2015-04-10 18:11:42 UTC) #6
mtklein
added a comment
5 years, 8 months ago (2015-04-13 17:18:30 UTC) #8
reed1
lgtm
5 years, 8 months ago (2015-04-13 18:05:11 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1081433002/20001
5 years, 8 months ago (2015-04-13 18:21:34 UTC) #11
commit-bot: I haz the power
5 years, 8 months ago (2015-04-13 19:17:08 UTC) #12
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/1dda2194e0c3a486032b2dd4d3eaea67fa47d818

Powered by Google App Engine
This is Rietveld 408576698