Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310123007:
Add support for light dumps in SkTraceMemoryDump interface. (Closed)
DescriptionAdd support for light dumps in SkTraceMemoryDump interface.
SkGlyphCache dumps too many glyphs, and to reduce the trace size,
RequestDetails enum is added to SkTraceMemoryDump interface. This
would tell caches to dump only requested details or not a detailed
dump.
BUG=499731
Committed: https://skia.googlesource.com/skia/+/f0c986503b982cfbd4d859573c11bc2a154b42f5
Patch Set 1 #Patch Set 2 : Add backing. #Patch Set 3 : Fix test. #
Total comments: 1
Patch Set 4 : Move to RequestedDetails enum. #Patch Set 5 : Nits. #
Total comments: 1
Patch Set 6 : uint. #
Total comments: 4
Patch Set 7 : Fixes. #Patch Set 8 : Nit. #Patch Set 9 : Test fix. #Patch Set 10 : Nit. #Patch Set 11 : Change to light and detailed. #
Total comments: 4
Patch Set 12 : Renames. #
Total comments: 3
Patch Set 13 : Fixes. #Patch Set 14 : Using KObjectsBreakdowns_LevelOfDetail. #
Total comments: 1
Patch Set 15 : Fixing typo. #
Messages
Total messages: 51 (16 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL if this is as per our discussion. Thanks
lgtm
ssid@chromium.org changed reviewers: + reed@google.com
PTAL, Thanks
this will break our unittests, as they have a subclass of SkTraceMemoryDump, that will need to be updated for the new virtual.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/40001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-09-11 01:21 UTC
Fixed the test, PTAL. Thanks
lgtm w/ naming question/comment. https://codereview.chromium.org/1310123007/diff/40001/include/core/SkTraceMem... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/40001/include/core/SkTraceMem... include/core/SkTraceMemoryDump.h:61: virtual bool isLightDump() const = 0; a little odd that the comment specifics (only memory totals) is not some how part of the name -- i.e. that this method *requires* a comment for callers (and implementors) to guess what it means. Do you anticipate future refinements? I wonder if soon a bitfield or other expansion will be desired: enum Required { kMemoryTotals = 1 << 0, kDiscardableDetails = 1 << 1, kOwnershipTracing = 1 << 2, // just making this one up ... }; virtual Required getRequiredDetails() const = 0; Just a thought.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:60001) has been deleted
LGTM, like reed@'s proposal Just mind the type https://codereview.chromium.org/1310123007/diff/100001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/100001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:73: virtual int getRequestedDetails() const = 0; I'd probably make this uint and fixed size (uint32_t)?
Thanks made changes.
https://codereview.chromium.org/1310123007/diff/120001/src/core/SkResourceCac... File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1310123007/diff/120001/src/core/SkResourceCac... src/core/SkResourceCache.cpp:692: SkTraceMemoryDump::kDiscardableDetails | SkTraceMemoryDump::kMallocDetails) { This flag check seems wrong. I think you meant here dump->getRequestedDetails() & (...kMemoryTotals | kDiscardableDetails ...) or dump->getRequestedDetails() & kMemoryTotals & kDiscardableDetails
https://codereview.chromium.org/1310123007/diff/120001/tests/TraceMemoryDumpT... File tests/TraceMemoryDumpTest.cpp (right): https://codereview.chromium.org/1310123007/diff/120001/tests/TraceMemoryDumpT... tests/TraceMemoryDumpTest.cpp:27: uint32_t getRequestedDetails() const override { return 0; } Does this mean our test dumper won't dump anything, since we return 0?
Fixed the mistakes. PTAL https://codereview.chromium.org/1310123007/diff/120001/src/core/SkResourceCac... File src/core/SkResourceCache.cpp (right): https://codereview.chromium.org/1310123007/diff/120001/src/core/SkResourceCac... src/core/SkResourceCache.cpp:692: SkTraceMemoryDump::kDiscardableDetails | SkTraceMemoryDump::kMallocDetails) { On 2015/09/11 17:39:53, Primiano Tucci wrote: > This flag check seems wrong. I think you meant here > dump->getRequestedDetails() & (...kMemoryTotals | kDiscardableDetails ...) > > or > > dump->getRequestedDetails() & kMemoryTotals & kDiscardableDetails Fixed this to & https://codereview.chromium.org/1310123007/diff/120001/tests/TraceMemoryDumpT... File tests/TraceMemoryDumpTest.cpp (right): https://codereview.chromium.org/1310123007/diff/120001/tests/TraceMemoryDumpT... tests/TraceMemoryDumpTest.cpp:27: uint32_t getRequestedDetails() const override { return 0; } On 2015/09/11 19:03:31, reed1 wrote: > Does this mean our test dumper won't dump anything, since we return 0? I thought this was just build test. True, adding some detail looks better instead of returning 0.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with just naming nits. thanks https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:27: kLightDumpRequest, Remove rquest from the name. Doesn't tell anything useful. I'd probably go just for kLight and kDetailed https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:70: * Returns the type of details requested in the dump. The skia objects are expected to dump THis is a bit difficult to read, but the content is good. I'd probably reword as The granularity of the dump is supposed to match the LevelOfDetails argument. The level of detail must not affect the total size reported, but only granularity of the child entries.
Done, Thanks https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:27: kLightDumpRequest, On 2015/09/24 13:10:27, Primiano Tucci wrote: > Remove rquest from the name. Doesn't tell anything useful. > I'd probably go just for kLight and kDetailed I didn't want to make it enum class. So, making it kLightDump. https://codereview.chromium.org/1310123007/diff/220001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:70: * Returns the type of details requested in the dump. The skia objects are expected to dump On 2015/09/24 13:10:27, Primiano Tucci wrote: > THis is a bit difficult to read, but the content is good. > I'd probably reword as > > The granularity of the dump is supposed to match the LevelOfDetails argument. > The level of detail must not affect the total size reported, but only > granularity of the child entries. Done.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@ Sorry for lot of changes, Could you please take a look now? Thanks for the patience.
https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:25: enum LevelOfDetails { naming nits: 1. skia would make the enum name singular: LevelOfDetail 2. skia uses the name as a suffix, to give the values a clearer scope. e.g. kLight_LevelOfDetail, kDetailed_LevelOfDetail, 3. Light seems like a find modifier for LeveOfDetail, but Detailed is not a clear. This is mostly bike-shedding, but I think we could put a little thought into what we want. - do we anticipate other levels in the future? - do we, perhaps, think a bitfield would be more useful/flexible? (e.g. kIncludeTotals, kIncludeDiscardable, etc.)
Ugh, my spelling was terrible. kLight seems like a fine modifier for Level-Of-Detail, but kDetailed not so much, since "Detail" is in the suffix already.
https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:25: enum LevelOfDetails { > - do we anticipate other levels in the future? Hmm not really. The only other level we'll have in chrome is an "heap profiling mode" (see crbug.com/524631 for design doc) where we'll record stack traces for malloc calls and similar. BUt since skia, at least today, already uses malloc, it would get it for free without needing to adding any extra here. > - do we, perhaps, think a bitfield would be more useful/flexible? (e.g. > kIncludeTotals, kIncludeDiscardable, etc.) The main problem here is that we want mainly to options: - quick and light (w.r.t trace size) dumps - "fat" dumps where we try to squeeze as much breakdowns as possible. the only restriction is that, regardless of the level of detail, the final total of bytes reported should not change (but one mode could report just 2 rows, the other 2000). The problem is that we can't really put malloc/discardable in the names because in some contexts we need to still report discardable details even in "light" dumps for the sake of getting the correct total. So we should keep it generic w.r.t malloc/discardable in the names. Maybe we could call it KObjectsBreakdowns_LevelOfDetail or something like that, to emphasize the fact that we try to squeeze out as much information as possible?
https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:25: enum LevelOfDetails { On 2015/09/28 17:02:00, reed1 wrote: > naming nits: > > 1. skia would make the enum name singular: LevelOfDetail > done. > 2. skia uses the name as a suffix, to give the values a clearer scope. > > e.g. > > kLight_LevelOfDetail, > kDetailed_LevelOfDetail, > done. > 3. Light seems like a find modifier for LeveOfDetail, but Detailed is not a > clear. This is mostly bike-shedding, but I think we could put a little thought > into what we want. > - do we anticipate other levels in the future? > - do we, perhaps, think a bitfield would be more useful/flexible? (e.g. > kIncludeTotals, kIncludeDiscardable, etc.) Maybe I can make it kHeavy?
On 2015/09/28 17:23:32, ssid wrote: > https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... > File include/core/SkTraceMemoryDump.h (right): > > https://codereview.chromium.org/1310123007/diff/240001/include/core/SkTraceMe... > include/core/SkTraceMemoryDump.h:25: enum LevelOfDetails { > On 2015/09/28 17:02:00, reed1 wrote: > > naming nits: > > > > 1. skia would make the enum name singular: LevelOfDetail > > > > done. > > 2. skia uses the name as a suffix, to give the values a clearer scope. > > > > e.g. > > > > kLight_LevelOfDetail, > > kDetailed_LevelOfDetail, > > > done. > > > 3. Light seems like a find modifier for LeveOfDetail, but Detailed is not a > > clear. This is mostly bike-shedding, but I think we could put a little thought > > into what we want. > > - do we anticipate other levels in the future? > > - do we, perhaps, think a bitfield would be more useful/flexible? (e.g. > > kIncludeTotals, kIncludeDiscardable, etc.) > > Maybe I can make it kHeavy? I have changed it to KObjectsBreakdowns_LevelOfDetail. PTAL, thanks.
I presume you mean kObject... and not KObject... lgtm other than that https://codereview.chromium.org/1310123007/diff/280001/include/core/SkTraceMe... File include/core/SkTraceMemoryDump.h (right): https://codereview.chromium.org/1310123007/diff/280001/include/core/SkTraceMe... include/core/SkTraceMemoryDump.h:30: KObjectsBreakdowns_LevelOfDetail K or k ?
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1310123007/#ps300001 (title: "Fixing typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310123007/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310123007/300001
Message was sent while issue was closed.
Committed patchset #15 (id:300001) as https://skia.googlesource.com/skia/+/f0c986503b982cfbd4d859573c11bc2a154b42f5
Message was sent while issue was closed.
robertphillips@google.com changed reviewers: + robertphillips@google.com
Message was sent while issue was closed.
This appears to be blocking the roll with several errors like:
../../skia/ext/skia_memory_dump_provider.cc:40:28: error: variable type
'skia::SkTraceMemoryDump_Chrome' is an abstract class
SkTraceMemoryDump_Chrome skia_dumper(process_memory_dump);
^
../../third_party/skia/include/core/SkTraceMemoryDump.h:74:27: note:
unimplemented pure virtual method 'getRequestedDetails' in
'SkTraceMemoryDump_Chrome'
virtual LevelOfDetail getRequestedDetails() const = 0;
Message was sent while issue was closed.
On 2015/09/30 11:56:28, robertphillips wrote: > This appears to be blocking the roll with several errors like: > > ../../skia/ext/skia_memory_dump_provider.cc:40:28: error: variable type > 'skia::SkTraceMemoryDump_Chrome' is an abstract class > SkTraceMemoryDump_Chrome skia_dumper(process_memory_dump); > ^ > ../../third_party/skia/include/core/SkTraceMemoryDump.h:74:27: note: > unimplemented pure virtual method 'getRequestedDetails' in > 'SkTraceMemoryDump_Chrome' > virtual LevelOfDetail getRequestedDetails() const = 0; This CL https://codereview.chromium.org/1324453008/ fixes the issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
