Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1583483002:
[tracing] Add method to create "weak" global dumps (Closed)
|
Created:
4 years, 11 months ago by ssid Modified:
4 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, darin-cc_chromium.org, jam, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Add method to create "weak" global dumps
The renderer process could add memory dumps that are known to be
non-existent in the browser. To resolve this issue, a notion of "weak"
global allocator dump is introduced so that the renderer could tell it
is unsure of the existence of the dump.
A weak dump will be removed in the UI unless at least one process
creates a non-weak global dump with the same guid. This would mark the
existence of the memory.
Sample use case: The browser creates a non-weak global dump and a
renderer process creates a weak global dump for discardable segments.
This represents the fact that the renderer doesn't know whether a
segment is or isn't present in memory.
BUG=570655
Committed: https://crrev.com/a937635dd0ffef780f0a0b5fbbeccd31cac5e60e
Cr-Commit-Position: refs/heads/master@{#370683}
Patch Set 1 #Patch Set 2 : Changes #Patch Set 3 : global is weak. #Patch Set 4 : Edges are weak #Patch Set 5 : back to weak dumps. #
Total comments: 17
Patch Set 6 : Just weak global dumps. #Patch Set 7 : unittest #Patch Set 8 : Nits. #Patch Set 9 : Nit. #
Total comments: 29
Patch Set 10 : petr's comments. #Patch Set 11 : Nits. #
Total comments: 10
Patch Set 12 : Fixes. #
Total comments: 22
Patch Set 13 : Fixes. #
Messages
Total messages: 40 (21 generated)
Description was changed from ========== [tracing] Add "weak" property to global allocator dumps The renderer could dump segments that are known to be non-existent from the browser. To fix this, "is_weak" property is added to global allocator dumps. If a global allocator dump is marked weak, then the associated segment in the process will not be accounted unless a strong global allocator dump exists in the same global memory dump. BUG=570655 ========== to ========== [tracing] Add "weak" property to ownership edges of allocator dumps The renderer could dump discardable segments that are known to be non-existent from the browser. To fix this, "is_weak" property is added to the ownership edges. If an ownership edge is marked weak then the source is made invalid if target doesn't exist. If an edge with global allocator dump is marked weak, then the associated segment in the process will not be accounted unless a dump strong edge with the global dump exists and lower importance exists. ==========
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL, thanks
ssid@chromium.org changed reviewers: + petrcermak@chromium.org
+petr I considered the suggestion of having a weak attribute on the global dump. But the tracing treats global dump just as an allocator dump, so i will be adding the property to allocator dumps. But, the MemoryAllocatorDump class itself does not know about ownership and edges. These are information in ProcessMemoryDump. So adding weak property to AllocatorDump is not explained clearly without the knowledge of edges and ownership. So feels more like it is a property of the edges.
Description was changed from ========== [tracing] Add "weak" property to ownership edges of allocator dumps The renderer could dump discardable segments that are known to be non-existent from the browser. To fix this, "is_weak" property is added to the ownership edges. If an ownership edge is marked weak then the source is made invalid if target doesn't exist. If an edge with global allocator dump is marked weak, then the associated segment in the process will not be accounted unless a dump strong edge with the global dump exists and lower importance exists. ========== to ========== [tracing] Add "weak" property to ownership edges of process memory dumps The renderer could dump discardable segments that are known to be non-existent from the browser. To fix this, "is_weak" property is added to the ownership edges. If an ownership edge is marked weak then the source is made invalid if target doesn't exist. If an edge with global allocator dump is marked weak, then the associated segment in the process will not be accounted unless a dump strong edge with the global dump exists and lower importance exists. ==========
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #5 (id:120001) has been deleted
Description was changed from ========== [tracing] Add "weak" property to ownership edges of process memory dumps The renderer could dump discardable segments that are known to be non-existent from the browser. To fix this, "is_weak" property is added to the ownership edges. If an ownership edge is marked weak then the source is made invalid if target doesn't exist. If an edge with global allocator dump is marked weak, then the associated segment in the process will not be accounted unless a dump strong edge with the global dump exists and lower importance exists. ========== to ========== [tracing] Add a "weak" flag to allocator dumps The renderer could dump segments that are known to be non-existent from the browser. To fix this "weak" flag is added to allocator dumps. Weak dumps are removed from accounting if not made strong. The global dump becomes strong if any process makes it strong. This flag is used by renderers to exclude purged segments. BUG=570655 ==========
I added flags to allocator dump as weak. WDYT?
A couple of comments. Description nits: * Please don't say "segments" in the description. They are specific to discardable memory and the flag is a generic concept. * s/"weak" flag is added/an optional "weak" flag can be added/ * s/to allocator dumps/to allocator dump/ * s/Weak dumps are removed/Weak dumps and their transitive owners and descendants are removed/ * You mention "strong" dumps, but don't explain what they are. In other words, "makes it strong" is completely unclear. * I suggest you move the first and last sentence to a separate paragraph where you explain the rationale of the flag (separate it from the paragraph explaining when a dump is weak and what the consequences are). Thanks, Petr https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:29: enum Flags { I think that this should be called "Flag" (singular, see https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=151) https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A weak dump is made invalid unless it is marked strong later, either by Please don't use the word "invalid" because we don't use it anywhere else and it is unclear what it means. You could say something along the lines of the dump being "ignored", "removed", ... https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A weak dump is made invalid unless it is marked strong later, either by It could also be 'marked' strong "earlier". https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:32: // All dumps that own an invalid dump and all its children will be marked nit: I'd say "All owners and children of a removed dump will also be removed transitively" https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:33: // invalid transitively. Default value is 0. Rather than "Default value is 0", say "A dump is strong by default". https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:73: void reset_flags(int flags) { flags_ &= !flags; } it this a naming convention? If not, "unset" might be less ambiguous. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:74: bool flags() const { return flags_; } this shouldn't be a bool... https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:99: int flags_; // See Flags. I think that this should be unsigned or Flag (even though you can assign combinations of flags to it) https://codereview.chromium.org/1583483002/diff/140001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1583483002/diff/140001/content/common/discard... content/common/discardable_shared_memory_heap.cc:431: global_dump->set_flags(base::trace_event::MemoryAllocatorDump::Flags::WEAK); This will work as long as there is no other user of the global dump within the process. However, even if there's another user of the global dump who created it first *without* the weak flag, the global dump will now be weak. We want the inverse semantics. To address this, I think that it would be better to add the following method: pmd->GetOrCreateWeakSharedGlobalAllocatorDump(shared_segment_guid); which would simply return an existing global dump (possibly strong) or create a new one and mark is as weak. On the other hand, the existing method: pmd->GetOrCreateSharedGlobalAllocatorDump(shared_segment_guid); would mark an existing global dump as strong and return it, or create a new one (and keep it marked as strong). The only issue here is combinatorial explosion of the method signatures. ssid, primiano: WDYT?
Description was changed from ========== [tracing] Add a "weak" flag to allocator dumps The renderer could dump segments that are known to be non-existent from the browser. To fix this "weak" flag is added to allocator dumps. Weak dumps are removed from accounting if not made strong. The global dump becomes strong if any process makes it strong. This flag is used by renderers to exclude purged segments. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be added in trace only if at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. So, the browser creates a non-weak global dump and renderer creates a weak global dump for discardable segments. BUG=570655 ==========
I added a new method to create weak global dump. PTAL Thanks https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:29: enum Flags { On 2016/01/13 17:46:14, petrcermak wrote: > I think that this should be called "Flag" (singular, see > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=151) Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A weak dump is made invalid unless it is marked strong later, either by On 2016/01/13 17:46:14, petrcermak wrote: > It could also be 'marked' strong "earlier". Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A weak dump is made invalid unless it is marked strong later, either by On 2016/01/13 17:46:14, petrcermak wrote: > Please don't use the word "invalid" because we don't use it anywhere else and it > is unclear what it means. You could say something along the lines of the dump > being "ignored", "removed", ... Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:32: // All dumps that own an invalid dump and all its children will be marked On 2016/01/13 17:46:14, petrcermak wrote: > nit: I'd say "All owners and children of a removed dump will also be removed > transitively" Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:33: // invalid transitively. Default value is 0. On 2016/01/13 17:46:14, petrcermak wrote: > Rather than "Default value is 0", say "A dump is strong by default". Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:73: void reset_flags(int flags) { flags_ &= !flags; } On 2016/01/13 17:46:14, petrcermak wrote: > it this a naming convention? If not, "unset" might be less ambiguous. Done. https://codereview.chromium.org/1583483002/diff/140001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:99: int flags_; // See Flags. On 2016/01/13 17:46:14, petrcermak wrote: > I think that this should be unsigned or Flag (even though you can assign > combinations of flags to it) Hm I made this integer because the traceValue accepts only integer or double. Not unsigned. Fixed and added a cast there. https://codereview.chromium.org/1583483002/diff/140001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1583483002/diff/140001/content/common/discard... content/common/discardable_shared_memory_heap.cc:431: global_dump->set_flags(base::trace_event::MemoryAllocatorDump::Flags::WEAK); On 2016/01/13 17:46:14, petrcermak wrote: > This will work as long as there is no other user of the global dump within the > process. However, even if there's another user of the global dump who created it > first *without* the weak flag, the global dump will now be weak. We want the > inverse semantics. To address this, I think that it would be better to add the > following method: > > pmd->GetOrCreateWeakSharedGlobalAllocatorDump(shared_segment_guid); > > which would simply return an existing global dump (possibly strong) or create a > new one and mark is as weak. On the other hand, the existing method: > > pmd->GetOrCreateSharedGlobalAllocatorDump(shared_segment_guid); > > would mark an existing global dump as strong and return it, or create a new one > (and keep it marked as strong). > > The only issue here is combinatorial explosion of the method signatures. > > ssid, primiano: WDYT? Done.
Thanks for the overhaul. I think that the patch is almost there :-) Petr https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A dump marked weak will be removed from tracing. It is used when dump nit: "... removed from tracing, unless it was/will be dumped by the same or a different process without the weak flag." https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:31: // provider is unsure of the existance of memory. s/existance/existence/ https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:31: // provider is unsure of the existance of memory. nit: s/memory/a block of memory/ (memory always exists) https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:39: uint8_t flags); As explained in my comment in ProcessMemoryDump, adding this constructor might not be necessary. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:139: // The weak flag is unset because this method should create non-weak dump. nit: s/non-weak/a non-weak/ https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:140: if (mad) I think that the following would be more readable: MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid); if (mad) { // Unset the weak flag of the existing dump (if applicable). mad->unset_flags(MemoryAllocatorDump::Flag::WEAK); return mad; } return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:151: mad = new MemoryAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), this, At this point, I wonder whether it wouldn't make more sense to drop the new MemoryAllocatorDump constructor (with the flags argument) and do the following here: if (!mad) { mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); mad->set_flags(MemoryAllocatorDump::Flag::WEAK); } return mad; Rationale: Make the source code of CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump more similar. In addition, similarly to the first method, I think that the following would be more readable: MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid); if (mad) return mad; mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); mad->set_flags(MemoryAllocatorDump::Flag::WEAK); return mad; One more thing, the CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid) could be factored out into a separate method (and shared between CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump), for example: MemoryAllocatorDump* ProcessMemoryDump::AddSharedGlobalAllocatorDumpInternal( const MemoryAllocatorDumpGuid& guid) { return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); } https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.h:102: // but with "WEAK" flag. A weak dump will be removed unless a non-weak dump nit: I'd drop the quotes and add an indefinite article in front of 'WEAK'. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.h:104: // (order of creation does not matter). All the owners and children of the nit: s/the owners/owners/ https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:33: MemoryAllocatorDumpGuid shared_mad_guid1(1); There are three test cases that you are missing: Create(NonWeak) + CreateWeak (in this particular order) -> non-weak Create(NonWeak) + Create(NonWeak) -> non-weak Create(Weak) + Create(Weak) -> non-weak https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:67: ASSERT_FALSE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); You should also check that the first MAD is not WEAK https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:121: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); You should also check that the first shared MAD is not WEAK https://codereview.chromium.org/1583483002/diff/220001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1583483002/diff/220001/content/common/discard... content/common/discardable_shared_memory_heap.cc:421: // The global dump is created as weak dump so that the segment is removed if nit: s/weak dump/a weak dump/
Thanks for modifying the description. A couple more comments: * Every dump will *always* be added to the trace (regardless of whether it is weak or not). It's the post-processing (Trace Viewer) that will remove it subsequently. * Add a blank line above "So, the browser [...]" * s/So, the browser/Sample use case: The browser process/ * s/renderer creates/a renderer process creates/ Petr
Description was changed from ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be added in trace only if at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. So, the browser creates a non-weak global dump and renderer creates a weak global dump for discardable segments. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. BUG=570655 ==========
Thanks, made changes. PTAL https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A dump marked weak will be removed from tracing. It is used when dump On 2016/01/18 11:23:46, petrcermak wrote: > nit: "... removed from tracing, unless it was/will be dumped by the same or a > different process without the weak flag." Hm, I think as far as mad is concerned, it means that a dump marked weak is removed. It can be marked strong later so that it is not removed. These cases are handled by pmd. Also "will be dumped without weak" would mean same dump can be dumped twice and is confusing to the user. The existing comment would prevent the user from using this flag directly from here instead of usign pmd to modify. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:31: // provider is unsure of the existance of memory. On 2016/01/18 11:23:46, petrcermak wrote: > nit: s/memory/a block of memory/ (memory always exists) Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:31: // provider is unsure of the existance of memory. On 2016/01/18 11:23:46, petrcermak wrote: > s/existance/existence/ Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:39: uint8_t flags); On 2016/01/18 11:23:46, petrcermak wrote: > As explained in my comment in ProcessMemoryDump, adding this constructor might > not be necessary. true. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:139: // The weak flag is unset because this method should create non-weak dump. On 2016/01/18 11:23:46, petrcermak wrote: > nit: s/non-weak/a non-weak/ Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:140: if (mad) On 2016/01/18 11:23:46, petrcermak wrote: > I think that the following would be more readable: > > MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid); > if (mad) { > // Unset the weak flag of the existing dump (if applicable). > mad->unset_flags(MemoryAllocatorDump::Flag::WEAK); > return mad; > } > return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:151: mad = new MemoryAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), this, On 2016/01/18 11:23:46, petrcermak wrote: > At this point, I wonder whether it wouldn't make more sense to drop the new > MemoryAllocatorDump constructor (with the flags argument) and do the following > here: > > if (!mad) { > mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > mad->set_flags(MemoryAllocatorDump::Flag::WEAK); > } > return mad; > > Rationale: Make the source code of CreateSharedGlobalAllocatorDump and > CreateWeakSharedGlobalAllocatorDump more similar. > > > In addition, similarly to the first method, I think that the following would be > more readable: > > MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid); > if (mad) > return mad; > mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > mad->set_flags(MemoryAllocatorDump::Flag::WEAK); > return mad; > > > One more thing, the CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), > guid) could be factored out into a separate method (and shared between > CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump), for > example: > > MemoryAllocatorDump* ProcessMemoryDump::AddSharedGlobalAllocatorDumpInternal( > const MemoryAllocatorDumpGuid& guid) { > return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > } Hm not sure if one line method is really needed. Used the second snippet you suggested. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.h:102: // but with "WEAK" flag. A weak dump will be removed unless a non-weak dump On 2016/01/18 11:23:46, petrcermak wrote: > nit: I'd drop the quotes and add an indefinite article in front of 'WEAK'. Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.h:104: // (order of creation does not matter). All the owners and children of the On 2016/01/18 11:23:46, petrcermak wrote: > nit: s/the owners/owners/ Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:33: MemoryAllocatorDumpGuid shared_mad_guid1(1); On 2016/01/18 11:23:46, petrcermak wrote: > There are three test cases that you are missing: > > Create(NonWeak) + CreateWeak (in this particular order) -> non-weak > Create(NonWeak) + Create(NonWeak) -> non-weak > Create(Weak) + Create(Weak) -> non-weak Done. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:67: ASSERT_FALSE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); On 2016/01/18 11:23:47, petrcermak wrote: > You should also check that the first MAD is not WEAK the new test checks this. This cheks only if the clear actually clears the weak flag. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:121: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); On 2016/01/18 11:23:46, petrcermak wrote: > You should also check that the first shared MAD is not WEAK added in new test. This only checks if weak flag is copied on takeDumps. https://codereview.chromium.org/1583483002/diff/220001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1583483002/diff/220001/content/common/discard... content/common/discardable_shared_memory_heap.cc:421: // The global dump is created as weak dump so that the segment is removed if On 2016/01/18 11:23:47, petrcermak wrote: > nit: s/weak dump/a weak dump/ Done.
LGTM with some final comments. Two more description comments: * s/with same guid/with the same guid/ * The sample use case should explain why the weak-ness is useful. I'd add a sentence like the following: "This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory." Thanks, Petr https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A dump marked weak will be removed from tracing. It is used when dump On 2016/01/18 16:28:35, ssid wrote: > On 2016/01/18 11:23:46, petrcermak wrote: > > nit: "... removed from tracing, unless it was/will be dumped by the same or a > > different process without the weak flag." > > Hm, I think as far as mad is concerned, it means that a dump marked weak is > removed. It can be marked strong later so that it is not removed. These cases > are handled by pmd. > Also "will be dumped without weak" would mean same dump can be dumped twice and > is confusing to the user. The existing comment would prevent the user from using > this flag directly from here instead of usign pmd to modify. Good point. My concern is that this suggests that the trace will NOT contain weak memory allocator dumps, which it WILL (they will be removed in Trace Viewer). What about "A dump marked as weak will be removed together with its transitive owners and descendants in trace post-processing"? https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:151: mad = new MemoryAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), this, On 2016/01/18 16:28:36, ssid wrote: > On 2016/01/18 11:23:46, petrcermak wrote: > > At this point, I wonder whether it wouldn't make more sense to drop the new > > MemoryAllocatorDump constructor (with the flags argument) and do the following > > here: > > > > if (!mad) { > > mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > > mad->set_flags(MemoryAllocatorDump::Flag::WEAK); > > } > > return mad; > > > > Rationale: Make the source code of CreateSharedGlobalAllocatorDump and > > CreateWeakSharedGlobalAllocatorDump more similar. > > > > > > In addition, similarly to the first method, I think that the following would > be > > more readable: > > > > MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid); > > if (mad) > > return mad; > > mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > > mad->set_flags(MemoryAllocatorDump::Flag::WEAK); > > return mad; > > > > > > One more thing, the > CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), > > guid) could be factored out into a separate method (and shared between > > CreateSharedGlobalAllocatorDump and CreateWeakSharedGlobalAllocatorDump), for > > example: > > > > MemoryAllocatorDump* ProcessMemoryDump::AddSharedGlobalAllocatorDumpInternal( > > const MemoryAllocatorDumpGuid& guid) { > > return CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid); > > } > > Hm not sure if one line method is really needed. Used the second snippet you > suggested. Acknowledged. https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/220001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:121: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); On 2016/01/18 16:28:36, ssid wrote: > On 2016/01/18 11:23:46, petrcermak wrote: > > You should also check that the first shared MAD is not WEAK > > added in new test. This only checks if weak flag is copied on takeDumps. The new test doesn't check that with regards to TakeAllDumpsFrom, so I would still vote for adding: ASSERT_FALSE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:36: pmd1->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid2); Weak -> Strong transition is already checked by your new test, so you could just drop this line. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:66: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); To be honest, you could probably check ASSERT_EQ(MemoryAllocatorDump::Flag::WEAK, shared_mad2()->flags()) since there are no other flags. You should also check: ASSERT_EQ(u0 /* non-weak */, shared_map1->flags()); https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:177: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); super-nit: I suggest you put this check after line 178 so that it's consistent with the shared_mad3...shared_mad5 blocks. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:179: auto shared_mad2 = pmd->GetSharedGlobalAllocatorDump(shared_mad_guid); I'd put a blank line above this line (so that each block starts with "auto shared_madN = ..." https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:180: ASSERT_EQ(shared_mad1, shared_mad2); nit: I'd add ASSERT_TRUE after this line as well. To summarize my comments in this test, I think that the following would be easier to follow and more visually pleasing: MemoryAllocatorDumpGuid shared_mad_guid(1); auto shared_mad1 = pmd->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid); ASSERT_EQ(shared_mad_guid, shared_mad1->guid()); ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); auto shared_mad2 = pmd->GetSharedGlobalAllocatorDump(shared_mad_guid); ASSERT_EQ(shared_mad1, shared_mad2); ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); auto shared_mad3 = pmd->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid); ASSERT_EQ(shared_mad1, shared_mad3); ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); ...
Description was changed from ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ==========
Description was changed from ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with the same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ==========
thanks made changes. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:36: pmd1->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid2); On 2016/01/18 17:18:12, petrcermak wrote: > Weak -> Strong transition is already checked by your new test, so you could just > drop this line. Done. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:66: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); On 2016/01/18 17:18:12, petrcermak wrote: > To be honest, you could probably check > ASSERT_EQ(MemoryAllocatorDump::Flag::WEAK, shared_mad2()->flags()) since there > are no other flags. > > You should also check: > > ASSERT_EQ(u0 /* non-weak */, shared_map1->flags()); Done. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:177: ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); On 2016/01/18 17:18:12, petrcermak wrote: > super-nit: I suggest you put this check after line 178 so that it's consistent > with the shared_mad3...shared_mad5 blocks. Done. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:179: auto shared_mad2 = pmd->GetSharedGlobalAllocatorDump(shared_mad_guid); On 2016/01/18 17:18:12, petrcermak wrote: > I'd put a blank line above this line (so that each block starts with "auto > shared_madN = ..." Done. https://codereview.chromium.org/1583483002/diff/260001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:180: ASSERT_EQ(shared_mad1, shared_mad2); On 2016/01/18 17:18:12, petrcermak wrote: > nit: I'd add ASSERT_TRUE after this line as well. > > To summarize my comments in this test, I think that the following would be > easier to follow and more visually pleasing: > > MemoryAllocatorDumpGuid shared_mad_guid(1); > auto shared_mad1 = pmd->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid); > ASSERT_EQ(shared_mad_guid, shared_mad1->guid()); > ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); > > auto shared_mad2 = pmd->GetSharedGlobalAllocatorDump(shared_mad_guid); > ASSERT_EQ(shared_mad1, shared_mad2); > ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad2->flags()); > > auto shared_mad3 = pmd->CreateWeakSharedGlobalAllocatorDump(shared_mad_guid); > ASSERT_EQ(shared_mad1, shared_mad3); > ASSERT_TRUE(MemoryAllocatorDump::Flag::WEAK & shared_mad1->flags()); > > ... Done.
LGTM with some minor comments https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.cc:95: value->SetInteger("flags", static_cast<int>(flags_)); and here you'd avoid the cast :) https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:29: enum Flag { s/Flag/Flags/ https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A dump marked weak will be removed from tracing. It is used when dump I'd clarify and say "will be discarded by TraceViewer" instead of "will be removed from tracing". I'd remove the "It is used when..." part as adds IMHO more confusion. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:78: void set_flags(uint8_t flags) { flags_ |= flags; } Why not just int (there seems to be lot of precedent in the codebase)? After all is not going to really save those 24 bits :) https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:79: void unset_flags(uint8_t flags) { flags_ &= ~flags; } s/unset/clear/ https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:97: uint8_t flags_; // See enum Flag. ditto here for int https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:140: // The weak flag is unset because this method should create a non-weak dump. s/unset/cleared/ https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.h:102: // but with a WEAK flag. A weak dump will be removed unless a non-weak dump s/removed/discarded/ also below https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.h:106: MemoryAllocatorDump* CreateWeakSharedGlobalAllocatorDump( You want to clarify that: The Weak flag applies only if: - No other non-weak dump, with the same GUID, exists already. Otherwise the dump stays non-weak. - No further non-weak dump with the same GUID is created. Otherwise the dump will become non-weak. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:64: ASSERT_EQ(0u /* non-weak */, shared_mad1->flags()); maybe you could have a Flags::DEFAULT = 0 in the enum? https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:173: TEST(ProcessMemoryDumpTest, CreateGlobalDumpTest) { s/CreateGlobalDUmpTest/GlobalAllocatorDumpTest/
Patchset #13 (id:300001) has been deleted
Patchset #13 (id:320001) has been deleted
Thanks, made changes. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.cc:95: value->SetInteger("flags", static_cast<int>(flags_)); On 2016/01/19 17:07:11, Primiano Tucci wrote: > and here you'd avoid the cast :) Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.h (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:29: enum Flag { On 2016/01/19 17:07:11, Primiano Tucci wrote: > s/Flag/Flags/ Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:30: // A dump marked weak will be removed from tracing. It is used when dump On 2016/01/19 17:07:11, Primiano Tucci wrote: > I'd clarify and say "will be discarded by TraceViewer" instead of "will be > removed from tracing". > I'd remove the "It is used when..." part as adds IMHO more confusion. Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:78: void set_flags(uint8_t flags) { flags_ |= flags; } On 2016/01/19 17:07:11, Primiano Tucci wrote: > Why not just int (there seems to be lot of precedent in the codebase)? After all > is not going to really save those 24 bits :) Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:79: void unset_flags(uint8_t flags) { flags_ &= ~flags; } On 2016/01/19 17:07:11, Primiano Tucci wrote: > s/unset/clear/ Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/memor... base/trace_event/memory_allocator_dump.h:97: uint8_t flags_; // See enum Flag. On 2016/01/19 17:07:11, Primiano Tucci wrote: > ditto here for int Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:140: // The weak flag is unset because this method should create a non-weak dump. On 2016/01/19 17:07:11, Primiano Tucci wrote: > s/unset/cleared/ Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.h:102: // but with a WEAK flag. A weak dump will be removed unless a non-weak dump On 2016/01/19 17:07:11, Primiano Tucci wrote: > s/removed/discarded/ also below Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump.h:106: MemoryAllocatorDump* CreateWeakSharedGlobalAllocatorDump( On 2016/01/19 17:07:11, Primiano Tucci wrote: > You want to clarify that: > The Weak flag applies only if: > - No other non-weak dump, with the same GUID, exists already. Otherwise the > dump stays non-weak. > - No further non-weak dump with the same GUID is created. Otherwise the dump > will become non-weak. Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... File base/trace_event/process_memory_dump_unittest.cc (right): https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:64: ASSERT_EQ(0u /* non-weak */, shared_mad1->flags()); On 2016/01/19 17:07:11, Primiano Tucci wrote: > maybe you could have a Flags::DEFAULT = 0 in the enum? Done. https://codereview.chromium.org/1583483002/diff/280001/base/trace_event/proce... base/trace_event/process_memory_dump_unittest.cc:173: TEST(ProcessMemoryDumpTest, CreateGlobalDumpTest) { On 2016/01/19 17:07:11, Primiano Tucci wrote: > s/CreateGlobalDUmpTest/GlobalAllocatorDumpTest/ Done.
ssid@chromium.org changed reviewers: + avi@chromium.org, reveman@chromium.org
+avi, +reveman for small change in content/discardable..heap. PTAL, thanks.
lgtm stamp
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/1583483002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583483002/340001
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 petrcermak@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1583483002/#ps340001 (title: "Fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1583483002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1583483002/340001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with the same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with the same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with the same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 ========== to ========== [tracing] Add method to create "weak" global dumps The renderer process could add memory dumps that are known to be non-existent in the browser. To resolve this issue, a notion of "weak" global allocator dump is introduced so that the renderer could tell it is unsure of the existence of the dump. A weak dump will be removed in the UI unless at least one process creates a non-weak global dump with the same guid. This would mark the existence of the memory. Sample use case: The browser creates a non-weak global dump and a renderer process creates a weak global dump for discardable segments. This represents the fact that the renderer doesn't know whether a segment is or isn't present in memory. BUG=570655 Committed: https://crrev.com/a937635dd0ffef780f0a0b5fbbeccd31cac5e60e Cr-Commit-Position: refs/heads/master@{#370683} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a937635dd0ffef780f0a0b5fbbeccd31cac5e60e Cr-Commit-Position: refs/heads/master@{#370683} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
