Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(224)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 7327047: Dusting off the code in #ifdef SK_DEBUG_SIZE

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 11 months ago by junov1
Modified:
12 years, 11 months ago
Reviewers:
caryclark, caryclark1, reed1
CC:
skia-review_googlegroups.com
Base URL:
http://skia.googlecode.com/svn/
Visibility:
Public.
Dusting off the code in #ifdef SK_DEBUG_SIZE This CL makes the SK_DEBUG_SIZE code usable again and adds support for bounding hierarchies to it. Also adding methods to query memory used by bounding box structures.

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 7
Created: 12 years, 11 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+170 lines, -97 lines) Patch
M trunk/src/core/SkBBoxHierarchy.h View 1 chunk +7 lines, -0 lines 0 comments Download
M trunk/src/core/SkPicturePlayback.h View 1 2 chunks +6 lines, -7 lines 0 comments Download
M trunk/src/core/SkPicturePlayback.cpp View 1 2 4 chunks +97 lines, -79 lines 5 comments Download
M trunk/src/core/SkPictureRecord.h View 1 chunk +1 line, -0 lines 0 comments Download
M trunk/src/core/SkPictureRecord.cpp View 1 4 chunks +30 lines, -11 lines 1 comment Download
M trunk/src/core/SkPictureStateTree.h View 1 chunk +2 lines, -0 lines 0 comments Download
M trunk/src/core/SkPictureStateTree.cpp View 1 chunk +4 lines, -0 lines 0 comments Download
M trunk/src/core/SkRTree.h View 1 chunk +5 lines, -0 lines 0 comments Download
M trunk/src/core/SkRTree.cpp View 1 chunk +4 lines, -0 lines 0 comments Download
M trunk/src/core/SkTileGrid.h View 1 chunk +6 lines, -0 lines 0 comments Download
M trunk/src/core/SkTileGrid.cpp View 1 chunk +8 lines, -0 lines 1 comment Download
Total messages: 3
|
junov1
PTAL
12 years, 11 months ago (2013年02月14日 20:17:25 UTC) #1
PTAL
Sign in to reply to this message.
reed1
Can we bracket the new virtuals with SK_DEBUG_SIZE (or whatever the right name for that ...
12 years, 11 months ago (2013年02月15日 09:45:08 UTC) #2
Can we bracket the new virtuals with SK_DEBUG_SIZE (or whatever the right name
for that flag should be), so we don't accidentally create some dependency on
those in our release code?
Adding Cary to help w/ the review, as the original author of size-dummping.
Sign in to reply to this message.
caryclark1
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlayback.cpp File trunk/src/core/SkPicturePlayback.cpp (right): https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlayback.cpp#newcode33 trunk/src/core/SkPicturePlayback.cpp:33: size_t overallBytes, bitmapBytes, matricesBytes, replace size_t with SkLONGLONG to ...
12 years, 11 months ago (2013年02月15日 13:27:53 UTC) #3
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
File trunk/src/core/SkPicturePlayback.cpp (right):
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
trunk/src/core/SkPicturePlayback.cpp:33: size_t overallBytes, bitmapBytes,
matricesBytes,
replace size_t with SkLONGLONG to remove casts below (adjusting all debug size
functions accordingly)
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
trunk/src/core/SkPicturePlayback.cpp:45: SkDebugf("bitmaps size %lld
(bitmaps:%d) ", (SkLONGLONG)bitmapBytes, bitmaps);
would be more clear to replace 'bitmaps size' with 'bitmap bytes', etc.
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
trunk/src/core/SkPicturePlayback.cpp:53: SkDebugf("boundingHierarchy %lld
(nodes:%d) ", (SkLONGLONG)boundingHierarchyBytes, boundingBoxes);
nodes, boundingBoxes : pick one
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
trunk/src/core/SkPicturePlayback.cpp:1008: SkOrderedWriteBuffer buffer(256,
storage, sizeof(storage));
what about overallocating storage and then asserting if written storage exceeds
sizeof(intptr_t) * 256?
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPicturePlay...
trunk/src/core/SkPicturePlayback.cpp:1025: // Approximation
is this comment a FIXME? a note? a problem? 
it would be great to know more of what you're documenting here
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPictureReco...
File trunk/src/core/SkPictureRecord.cpp (right):
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkPictureReco...
trunk/src/core/SkPictureRecord.cpp:852: result += sizeof(*fBitmapHeap) +
fBitmapHeap->bytesAllocated();
result = ...
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkTileGrid.cpp
File trunk/src/core/SkTileGrid.cpp (right):
https://codereview.appspot.com/7327047/diff/5001/trunk/src/core/SkTileGrid.cp...
trunk/src/core/SkTileGrid.cpp:29: size_t totalBytes = sizeof(*this) +
sizeof(SkTDArray<void *>) * fTileCount;
this looks suspicious. I don't see how this can know how much space the
SkTDArray uses.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /