Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1911643002:
Add configurable limit to allocations in heap profiler. (Closed)
|
Created:
4 years, 8 months ago by Maria Modified:
4 years, 7 months ago CC:
ssid, DmitrySkiba, chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd configurable limit to allocations in heap profiler.
Adds a new trace config parameter, min_allocation_size_bytes, which determines
the cutoff for allocation sizes below which the traces for that allocation are
excluded from the trace file.
Makes the default cutoff to be 1024 bytes.
BUG=605298
Committed: https://crrev.com/3b1e75900d721b57351754318af8bcfaa7815744
Cr-Commit-Position: refs/heads/master@{#391773}
Patch Set 1 #Patch Set 2 : Formatting changes #
Total comments: 14
Patch Set 3 : Fix cast style #
Total comments: 3
Patch Set 4 : Addressing comments #
Total comments: 6
Patch Set 5 : Moving to use session state #
Total comments: 30
Patch Set 6 : Addressing comments #Patch Set 7 : Fixed tracing handler unittest #
Total comments: 4
Patch Set 8 : Fixed naming in protocol.json #
Total comments: 10
Patch Set 9 : Addressed comments #Patch Set 10 : Update rename in unittest #
Total comments: 2
Patch Set 11 : Remove memory dump config details from devtools #Patch Set 12 : unittest fix #Patch Set 13 : Unittest changes #Patch Set 14 : Whitespace change #Messages
Total messages: 48 (16 generated)
mariakhomenko@chromium.org changed reviewers: + primiano@chromium.org
caseq@chromium.org changed reviewers: + caseq@chromium.org
https://codereview.chromium.org/1911643002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/40001/base/trace_event/trace_... base/trace_event/trace_config.cc:524: if (!memory_dump_config.GetInteger(kMinAllocationSize, &min_size_bytes)) { Please also update MemoryDumpConfig in devtools' protocol.json (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) Also, please update the tests (src/content/browser/devtools/protocol/tracing_handler_unittest.cc and, perhaps, src/base/trace_event/trace_config_unittest.cc) https://codereview.chromium.org/1911643002/diff/40001/base/trace_event/trace_... base/trace_event/trace_config.cc:527: if (min_size_bytes >= 0) merge into the condition above?
ssid@chromium.org changed reviewers: + ssid@chromium.org
Please update the trace_config_unittest.cc. I know this code is never used. But it will good to have this handy if needed in future. third_party/catapult/telemetry/telemetry/timeline/tracing_config.py: class MemoryDumpConfig. If you have time please change this as well. Thanks. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:136: size_t minSizeBytes) { Wait, isn't this supposed to be min_size_in_bytes in base/ code? why is style changed https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:189: const TraceConfig trace_config = TraceConfig& will reduce a copy? https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:100: // Minimum size of an allocation for which a stack trace will be kept. It is not just stack trace, it could be type buckets as well. maybe "for which an allocation bucket will be broken down with children"? https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:53: const char kMinAllocationSize[] = "min_allocation_size_bytes"; heap_profiler_min_bucket_size. Since min allocation size should be specified only with heap profiling enabled and allocation size does not tell much in trace config. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:527: if (min_size_bytes >= 0) merge with above else, reduces one block. Also this could be made: if (memory_dump_config.GetInteger(kMinAllocationSize, &min_size_bytes) && min_size_bytes) { min_allocation_size_bytes_ = min_size_bytes. } else { min_allocation_size_bytes_ = kDefaultMinAllocationSizeBytes } OR: if (memory_dump_config.GetInteger(kMinAllocationSize, &min_size_bytes)) { DCHECK(min_size_bytes); min_allocation_size_bytes_ = min_size_bytes. } else { min_allocation_size_bytes_ = kDefaultMinAllocationSizeBytes } https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:599: memory_dump_config->Set(kTriggersParam, std::move(triggers_list)); You should add a line here: memory_dump_config->Set(kMinAllocationSize, min_allocation_size_bytes_); https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.h:248: size_t min_allocation_size_bytes_; min_bucket_size_in_bytes_? allocation size sounds like a limit on size of allocations recorded, rather than a limit to stop breaking down. primiano@ will suggest better names!
The description is misleading: /s/config parameter/trace config parameter. "cutoff for allocation sizes that we keep around" means the allocations below this size is discarded from heap profiler. But it is not true. The AllocationRegister keeps around all allocations made in chrome. The group of allocations which can't add up to the limit is excluded when adding to trace file only. Here the group means a bucket which has same allocation context. https://codereview.chromium.org/1911643002/diff/40001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/40001/base/trace_event/trace_... base/trace_event/trace_config.h:196: static const int kDefaultMinAllocationSizeBytes = 1024; 1024 might be a very low value. It could make the trace huge. Maybe we should see how many dumps can fit in trace file after this change and before this change in a similar test.
Description was changed from ========== Add configurable limit to allocations in heap profiler. Adds a new config parameter, min_allocation_size_bytes, which determines the cutoff for allocation sizes that we keep around. Makes the default cutoff to be 1024 bytes. BUG=605298 ========== to ========== Add configurable limit to allocations in heap profiler. Adds a new trace config parameter, min_allocation_size_bytes, which determines the cutoff for allocation sizes below which the traces for that allocation are excluded from the trace file. Makes the default cutoff to be 1024 bytes. BUG=605298 ==========
Thanks for your comments so far. I am still working on getting the unit tests running -- will finish it up tomorrow. Primiano, let me know if you have an opinion on what the parameter should be called. I am happy to rename it if needed. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:136: size_t minSizeBytes) { On 2016/04/20 23:00:30, ssid wrote: > Wait, isn't this supposed to be min_size_in_bytes in base/ code? why is style > changed breakBy got me confused :) fixed. I might do a separate CL to fix breakBy as well. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:189: const TraceConfig trace_config = On 2016/04/20 23:00:30, ssid wrote: > TraceConfig& will reduce a copy? I'll just inline this one https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:100: // Minimum size of an allocation for which a stack trace will be kept. On 2016/04/20 23:00:30, ssid wrote: > It is not just stack trace, it could be type buckets as well. maybe "for which > an allocation bucket will be broken down with children"? Done. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:53: const char kMinAllocationSize[] = "min_allocation_size_bytes"; On 2016/04/20 23:00:30, ssid wrote: > heap_profiler_min_bucket_size. > Since min allocation size should be specified only with heap profiling enabled > and allocation size does not tell much in trace config. Acknowledged. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:527: if (min_size_bytes >= 0) On 2016/04/20 23:00:30, ssid wrote: > merge with above else, reduces one block. > > Also this could be made: > if (memory_dump_config.GetInteger(kMinAllocationSize, &min_size_bytes) && > min_size_bytes) { > min_allocation_size_bytes_ = min_size_bytes. > } else { > min_allocation_size_bytes_ = kDefaultMinAllocationSizeBytes > } > > > OR: > if (memory_dump_config.GetInteger(kMinAllocationSize, &min_size_bytes)) { > DCHECK(min_size_bytes); > min_allocation_size_bytes_ = min_size_bytes. > } else { > min_allocation_size_bytes_ = kDefaultMinAllocationSizeBytes > } Done. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.cc:599: memory_dump_config->Set(kTriggersParam, std::move(triggers_list)); On 2016/04/20 23:00:30, ssid wrote: > You should add a line here: > memory_dump_config->Set(kMinAllocationSize, min_allocation_size_bytes_); Done. https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/20001/base/trace_event/trace_... base/trace_event/trace_config.h:248: size_t min_allocation_size_bytes_; On 2016/04/20 23:00:30, ssid wrote: > min_bucket_size_in_bytes_? allocation size sounds like a limit on size of > allocations recorded, rather than a limit to stop breaking down. primiano@ will > suggest better names! Acknowledged.
Thanks a lot for working on this Maria For the name I'd go for breakdown_threshold_bytes more comments below. https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:189: min_allocation_size_bytes_ = TraceLog::GetInstance()->GetCurrentTraceConfig() Hmm this is now introducing the assumption that HeapDumpWriter is alive only when tracing is enabled, which might cause bugs. Can we instead pass a HeapProfilerConfig& (see comment below) here? What I would do here is: It is safe to get the TraceLog in the OnMemoryDump of the MemoryDumpProviders, because at that point tracing is guaranteed to be active. This will mean having to do GetCurrentTraceConfig() in 3 places (malloc, partition alloc and blinkgc heap profiler), which is bad in terms of copy/paste but better in terms of lifetime. A better alternative could be the following: in MemoryDumpManager::OnTraceLogEnabled(), we already read the MemoryDumpConfig from the TraceLog. Let's copy that into the SessionState (which is accessible to all MemoryDumpProviders via pmd->session_state()). If you do that, at this point the only arg you need to pass to HeapDumpWriter is a const MemoryDumpSessionState&, which would make everything cleaner. In other words today in malloc_dump_provider you have: std::unique_ptr<TracedValue> heap_dump = ExportHeapDump( metrics_by_context, pmd->session_state()->stack_frame_deduplicator(), pmd->session_state()->type_name_deduplicator()); with this change you will just std::unique_ptr<TracedValue> heap_dump = ExportHeapDump( metrics_by_context, pmd->session_state()) This can be done in a separate CL, before or after this (i.e. you could first refactor the existing code to move make this take the SessionState which is going to be a non-funcitonal refactor easier to review, and later add the extra threshold to the trace config) https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.h (right): https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.h:102: size_t min_allocation_size_bytes_; breakdown_threshold_bytes_? (to match the name of BreakDown function) https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/trace_... base/trace_event/trace_config.h:50: typedef std::vector<MemoryDumpTriggerConfig> MemoryDumpConfig; I think this is the culprit. Today typedef std::vector<MemoryDumpTriggerConfig> MemoryDumpConfig, which is essentially assuming MemoryDumpConfig contains only the list of triggers. I think the way to go is making this like: struct MemoryDumpConfig { std::vector<MemoryDumpTriggerConfig> triggers; HeapProfilerConfig heap_profiler_config; } where struct HeapProfilerConfig { uint32_t breakdown_threshold_bytes; } does it make sense? https://codereview.chromium.org/1911643002/diff/60001/base/trace_event/trace_... base/trace_event/trace_config.h:150: size_t GetMinAllocationSizeBytes() const { I think this should be just part of MemoryDumpConfig (for which there is already a getter below on line 178). No need to expose this here. https://codereview.chromium.org/1911643002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4701: { "name": "min_allocation_size_bytes", "type": "number", "optional": true, "description": "Provides a cut off value, if a trace allocates less than that, it is discarded." } I would probably not bother exposing this to the devtools protocol, unless we have a need for it.
Still working on unit tests. Primiano, can you take a look at non-test stuff? I want to make sure it looks ok. I wasn't entirely certain about object ownership in passing session state.
Everything makes sense % one minor thing in the internal ctor of HeapDumpWriter. My only other comments are about keeping the memory-infra stuff in TraceConfig more self contained. The rest is all great. Thanks a lot for this! https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:76: // Groups the allocations in the bucket by |break_by|. The buckets in the nice thanks for fixing also nits in existing code :) https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:135: // more than |minSizeBytes| to the total size. The long tail is omitted. s/minSizeBytes/min_size_bytes/ https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:187: HeapDumpWriter::HeapDumpWriter(const MemoryDumpSessionState& session_state) Since this is an internal class, here I'd keep the explicit pointers being passed as argument (sorry I realized only now that my comment was a bit misplaced and should have referred to the public method ExtraceHeapDump below). Instead the change below to ExtractHeapDump is fine The rationale is: if you take the pointers here out of a const& reference, there is the risk that the SessionState (that owns the deuplicators) can be destroyed while this class is still alive. In this specific case will work, because the ExtractHeapDump is the one that makes sure that HeapDumpWriter does not outlive the SessionState being passed. BUt in this piece of code is not explicit and you should not rely on it (instead it's fine to do that in ExtractHeapDump, see my comment there). https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:318: internal::HeapDumpWriter writer(session_state); Here instead is perfectly reasonable. This is the place where you should extract the * pointers and pass them to the HeapDumpWriter. Everything is consistent here because the code guarantees that the HeapDUmpWriter gets destroyed before you return, so it can't possibly outlive the SessionState that you get passed. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:217: auto session = new MemoryDumpSessionState(); This is leaking the SessionState now, there is no delete. I guess here you want: unique_ptr<MemoryDumpSessionState> session(new MemoryDumpSessionState()) https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:220: HeapDumpWriter writer(*session); anyways, following my suggestions above, since this is testing the internal class, there is no need to change this. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:287: auto session = new MemoryDumpSessionState(); ditto about leaking, and ditto about might not be needed https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:198: metrics_by_context, *pmd->session_state()); yes fantastic :) https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:658: std::vector<TraceConfig::MemoryDumpTriggerConfig> triggers_list = can this still be a const& reference instead of copying the vector? I mean const std::vector....& triggers_list https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_session_state.h (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:43: TraceConfig::MemoryDumpConfig memory_dump_config() const { I'd return a const TraceConfig::MemoryDumpConfig& here. No need to return a copy https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:47: void SetMemoryDumpConfig(TraceConfig::MemoryDumpConfig config); make the argument a const& ref so you avoid doing an extra copy just for the method call. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:61: // A set of configurations for this session. I'd say: The memory dump config, copied at the time when the tracing session was started. or something similar. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.cc:519: } I'd just do everything in this method, just to avoid confusion in TraceCOnfig. It's a shared place and is not just about memory-infra, so would keep our stuff self contained. Also there seems that you never have the need of calling any of the sub-setters, right? https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.h:44: struct MemoryDumpTriggerConfig { I think just for the sake of legibility these structs (this and HeapProfilerOptions) be inner structs of MemoryDumpConfig, so it's clear they all belong to the same thing. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.h:240: void SetTriggers(const base::DictionaryValue& memory_dump_config); why do we have these intermediate setters (SetTriggers and SetHeapProfilerOptions)? Can we have just the SetMemoryDUmpConfig?
PTAL https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:76: // Groups the allocations in the bucket by |break_by|. The buckets in the On 2016/04/22 14:18:28, Primiano Tucci wrote: > nice thanks for fixing also nits in existing code :) Acknowledged. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:135: // more than |minSizeBytes| to the total size. The long tail is omitted. On 2016/04/22 14:18:28, Primiano Tucci wrote: > s/minSizeBytes/min_size_bytes/ Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:187: HeapDumpWriter::HeapDumpWriter(const MemoryDumpSessionState& session_state) On 2016/04/22 14:18:28, Primiano Tucci wrote: > Since this is an internal class, here I'd keep the explicit pointers being > passed as argument (sorry I realized only now that my comment was a bit > misplaced and should have referred to the public method ExtraceHeapDump below). > Instead the change below to ExtractHeapDump is fine > > The rationale is: if you take the pointers here out of a const& reference, there > is the risk that the SessionState (that owns the deuplicators) can be destroyed > while this class is still alive. > > In this specific case will work, because the ExtractHeapDump is the one that > makes sure that HeapDumpWriter does not outlive the SessionState being passed. > BUt in this piece of code is not explicit and you should not rely on it (instead > it's fine to do that in ExtractHeapDump, see my comment there). > Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer.cc:318: internal::HeapDumpWriter writer(session_state); On 2016/04/22 14:18:28, Primiano Tucci wrote: > Here instead is perfectly reasonable. > This is the place where you should extract the * pointers and pass them to the > HeapDumpWriter. > Everything is consistent here because the code guarantees that the > HeapDUmpWriter gets destroyed before you return, so it can't possibly outlive > the SessionState that you get passed. Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_heap_dump_writer_unittest.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:217: auto session = new MemoryDumpSessionState(); On 2016/04/22 14:18:28, Primiano Tucci wrote: > This is leaking the SessionState now, there is no delete. > I guess here you want: > unique_ptr<MemoryDumpSessionState> session(new MemoryDumpSessionState()) Acknowledged. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:220: HeapDumpWriter writer(*session); On 2016/04/22 14:18:29, Primiano Tucci wrote: > anyways, following my suggestions above, since this is testing the internal > class, there is no need to change this. Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_heap_dump_writer_unittest.cc:287: auto session = new MemoryDumpSessionState(); On 2016/04/22 14:18:29, Primiano Tucci wrote: > ditto about leaking, and ditto about might not be needed Acknowledged. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/malloc... File base/trace_event/malloc_dump_provider.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/malloc... base/trace_event/malloc_dump_provider.cc:198: metrics_by_context, *pmd->session_state()); On 2016/04/22 14:18:29, Primiano Tucci wrote: > yes fantastic :) Acknowledged. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:658: std::vector<TraceConfig::MemoryDumpTriggerConfig> triggers_list = On 2016/04/22 14:18:29, Primiano Tucci wrote: > can this still be a const& reference instead of copying the vector? > I mean const std::vector....& triggers_list Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_session_state.h (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:43: TraceConfig::MemoryDumpConfig memory_dump_config() const { On 2016/04/22 14:18:29, Primiano Tucci wrote: > I'd return a const TraceConfig::MemoryDumpConfig& here. No need to return a copy Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:47: void SetMemoryDumpConfig(TraceConfig::MemoryDumpConfig config); On 2016/04/22 14:18:29, Primiano Tucci wrote: > make the argument a const& ref so you avoid doing an extra copy just for the > method call. Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_session_state.h:61: // A set of configurations for this session. On 2016/04/22 14:18:29, Primiano Tucci wrote: > I'd say: The memory dump config, copied at the time when the tracing session was > started. > or something similar. Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.cc:519: } On 2016/04/22 14:18:29, Primiano Tucci wrote: > I'd just do everything in this method, just to avoid confusion in TraceCOnfig. > It's a shared place and is not just about memory-infra, so would keep our stuff > self contained. > Also there seems that you never have the need of calling any of the sub-setters, > right? Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.h:44: struct MemoryDumpTriggerConfig { On 2016/04/22 14:18:29, Primiano Tucci wrote: > I think just for the sake of legibility these structs (this and > HeapProfilerOptions) be inner structs of MemoryDumpConfig, so it's clear they > all belong to the same thing. Done. https://codereview.chromium.org/1911643002/diff/80001/base/trace_event/trace_... base/trace_event/trace_config.h:240: void SetTriggers(const base::DictionaryValue& memory_dump_config); On 2016/04/22 14:18:29, Primiano Tucci wrote: > why do we have these intermediate setters (SetTriggers and > SetHeapProfilerOptions)? > Can we have just the SetMemoryDUmpConfig? I found it more clean to do it this way rather than nest the code inside checks, but I don't care too much. Combined the methods.
https://codereview.chromium.org/1911643002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/protocol.json:4701: { "name": "min_allocation_size_bytes", "type": "number", "optional": true, "description": "Provides a cut off value, if a trace allocates less than that, it is discarded." } On 2016/04/21 16:53:22, Primiano Tucci wrote: > I would probably not bother exposing this to the devtools protocol, unless we > have a need for it. Well, it gets exposed automatically since we're reusing the trace config parser. So it's just a matter of whether we document it here or not. I've no strong feelings towards wither way, but I think we eventually need to get chrome://tracing wired through the DevTools protocol rather than DOM UI bindings, as it's already doing this in case of remote tracing. https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4699: { "name": "breakdown_threshold_bytes", "type": "number", "optional": true, "description": "The threshold in bytes beneath which heap profiler stacks are not broken down any further" } please note the style of identifiers in protocol.json is that of WebKit/blink originally, i.e. this should be breakdownThresholdBytes. ConvertDictKeyStyle() in tracing_handler.cc will take care of converting this to unix_style for you. https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4708: { "name": "heap_profiler_options", "$ref": "HeapProfilerOptions", "optional": true, "description": "Provides additional options for the heap profiler." } ditto.
Andrey, I am getting the following errors on upload from devtools presubmit: JSDoc validator output: Exception in thread "main" java.lang.UnsupportedClassVersionError: org/chromium/devtools/jsdoc/JsDocValidator : Unsupported major.minor version 52.0 at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:803) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) at java.net.URLClassLoader.defineClass(URLClassLoader.java:449) at java.net.URLClassLoader.access$100(URLClassLoader.java:71) at java.net.URLClassLoader$1.run(URLClassLoader.java:361) at java.net.URLClassLoader$1.run(URLClassLoader.java:355) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:354) at java.lang.ClassLoader.loadClass(ClassLoader.java:425) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308) at java.lang.ClassLoader.loadClass(ClassLoader.java:358) at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:482) Modular compilation output: Exception in thread "main" java.lang.UnsupportedClassVersionError: org/chromium/devtools/compiler/Runner : Unsupported major.minor version 52.0 at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:803) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) at java.net.URLClassLoader.defineClass(URLClassLoader.java:449) at java.net.URLClassLoader.access$100(URLClassLoader.java:71) at java.net.URLClassLoader$1.run(URLClassLoader.java:361) at java.net.URLClassLoader$1.run(URLClassLoader.java:355) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:354) at java.lang.ClassLoader.loadClass(ClassLoader.java:425) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308) at java.lang.ClassLoader.loadClass(ClassLoader.java:358) at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:482) I believe this is due to the presubmit being compiled with Java 8, while I am running Java 7. Currently I am just bypassing this, but may be nice to have this fixed. Not sure who maintains these... https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4699: { "name": "breakdown_threshold_bytes", "type": "number", "optional": true, "description": "The threshold in bytes beneath which heap profiler stacks are not broken down any further" } On 2016/04/26 01:15:07, caseq wrote: > please note the style of identifiers in protocol.json is that of WebKit/blink > originally, i.e. this should be breakdownThresholdBytes. ConvertDictKeyStyle() > in tracing_handler.cc will take care of converting this to unix_style for you. Done. https://codereview.chromium.org/1911643002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4708: { "name": "heap_profiler_options", "$ref": "HeapProfilerOptions", "optional": true, "description": "Provides additional options for the heap profiler." } On 2016/04/26 01:15:07, caseq wrote: > ditto. Done.
Awesome! //base/trace_event LGTM with some minor comments. Thanks a lot for doing this Maria! Good luck with Java presubmits :) https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:550: && min_size_bytes >= 0) { are you sure you want the && min_size_bytes >= 0 part? If I read the code correctly, doing this means that threshold = 0 --> threadshold = 1024 (the default value). But I thought you wanted a way to disable the threshold at all in some cases. https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:563: memory_dump_config_.triggers.push_back(kDefaultLightMemoryDumpTrigger); don't you want also to set kDefaultBreakdownThresholdBytes here, so that if somebody starts with --enable-heap-profiling , we get the default threshold? https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.h:51: struct MemoryDumpTriggerConfig { I'd probably shorten these names (e.g. s/MemoryDumpTriggerConfig/Trigger/ and s/HeapProfilerOptions/HeapProfiler/) and avoid the typedefs below, so you directly use MemoryDumpConfig::Trigger and MemoryDumpConfig::HeapProfiler in the .cc file. typedef IMHO hurt a bit readability, as it force people do read the code to jump through an extra hop. https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.h:62: ~HeapProfilerOptions(); I think you can omit empty dtors for trivial structures like this.
https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:550: && min_size_bytes >= 0) { On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > are you sure you want the && min_size_bytes >= 0 part? > If I read the code correctly, doing this means that threshold = 0 --> > threadshold = 1024 (the default value). > But I thought you wanted a way to disable the threshold at all in some cases. Sorry, maybe I am not seeing something, but my reading is if 0 is specified, then min_size_bytes >= 0 test succeeds and we go into if branch and set breakdown_threshold_bytes to 0, which will then be used. What am I missing? https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:563: memory_dump_config_.triggers.push_back(kDefaultLightMemoryDumpTrigger); On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > don't you want also to set kDefaultBreakdownThresholdBytes here, so that if > somebody starts with --enable-heap-profiling , we get the default threshold? memory_dump_config_.Clear() does that https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... File base/trace_event/trace_config.h (right): https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.h:51: struct MemoryDumpTriggerConfig { On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > I'd probably shorten these names (e.g. s/MemoryDumpTriggerConfig/Trigger/ and > s/HeapProfilerOptions/HeapProfiler/) and avoid the typedefs below, so you > directly use > > MemoryDumpConfig::Trigger and MemoryDumpConfig::HeapProfiler in the .cc file. > typedef IMHO hurt a bit readability, as it force people do read the code to jump > through an extra hop. Done. https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.h:62: ~HeapProfilerOptions(); On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > I think you can omit empty dtors for trivial structures like this. Done.
mariakhomenko@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
+dgozman, pfeldman for Blink approvals
The CQ bit was checked by mariakhomenko@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/1911643002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911643002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mariakhomenko@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/1911643002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911643002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
primiano@chromium.org changed reviewers: + haraken@chromium.org
still lgtm. +haraken for the trivial changes in the Web mem dump adaptor. https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:550: && min_size_bytes >= 0) { On 2016/04/28 17:23:39, Maria wrote: > On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > > are you sure you want the && min_size_bytes >= 0 part? > > If I read the code correctly, doing this means that threshold = 0 --> > > threadshold = 1024 (the default value). > > But I thought you wanted a way to disable the threshold at all in some cases. > > Sorry, maybe I am not seeing something, but my reading is if 0 is specified, > then min_size_bytes >= 0 test succeeds and we go into if branch and set > breakdown_threshold_bytes to 0, which will then be used. What am I missing? oops sorry somehow I didn't see the "=" part of the equation . ignore my comment. https://codereview.chromium.org/1911643002/diff/140001/base/trace_event/trace... base/trace_event/trace_config.cc:563: memory_dump_config_.triggers.push_back(kDefaultLightMemoryDumpTrigger); On 2016/04/28 17:23:39, Maria wrote: > On 2016/04/28 07:33:45, Primiano Tucci (traveling) wrote: > > don't you want also to set kDefaultBreakdownThresholdBytes here, so that if > > somebody starts with --enable-heap-profiling , we get the default threshold? > > memory_dump_config_.Clear() does that Acknowledged.
https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4704: { "id": "HeapProfiler", This is getting too hairy. Could we bring some sanity into the API? We are to support and maintain it.
Source/platform/ LGTM
https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:4704: { "id": "HeapProfiler", On 2016/04/29 02:14:27, pfeldman wrote: > This is getting too hairy. Could we bring some sanity into the API? We are to > support and maintain it. Can we delay the exposure of this to the devtools protocol API until the heap profiler changes are stabilized? There are lot of changes on flight (dskiba@ recently introduced native heap profiling mode, which would be yet another flag). I think in ~1-2 months from now we'd be in the state of making a more conscious decision about what we want to expose and allow us to model a more civilized API.
On 2016/04/29 08:54:01, Primiano Tucci (traveling) wrote: > https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/protocol.json (right): > > https://codereview.chromium.org/1911643002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/protocol.json:4704: { "id": "HeapProfiler", > On 2016/04/29 02:14:27, pfeldman wrote: > > This is getting too hairy. Could we bring some sanity into the API? We are to > > support and maintain it. > > Can we delay the exposure of this to the devtools protocol API until the heap > profiler changes are stabilized? > There are lot of changes on flight (dskiba@ recently introduced native heap > profiling mode, which would be yet another flag). > I think in ~1-2 months from now we'd be in the state of making a more conscious > decision about what we want to expose and allow us to model a more civilized > API. Pavel, I'd appreciate if you can provide some actionable next steps. I am happy to: 1) Remove API documentation from devtools 2) Make specific API changes that are reasonable in scope I cannot rewrite all of the existing trace config code, so that things stop being hairy. I believe that's what Primiano is doing for the next 6 months.
> Pavel,
>
> I'd appreciate if you can provide some actionable next steps. I am happy to:
> 1) Remove API documentation from devtools
> 2) Make specific API changes that are reasonable in scope
>
> I cannot rewrite all of the existing trace config code, so that things stop
> being hairy. I believe that's what Primiano is doing for the next 6 months.
I think making MemoryDumpConfig an opaque object before its structure settles
would unblock you:
{
"id": "MemoryDumpConfig",
"type": "object",
"description": "Memory config."
},
That would be represented as a base::DictionaryValue in code that you would be
able to parse manually.
On 2016/05/02 17:26:28, pfeldman wrote:
> > Pavel,
> >
> > I'd appreciate if you can provide some actionable next steps. I am happy to:
> > 1) Remove API documentation from devtools
> > 2) Make specific API changes that are reasonable in scope
> >
> > I cannot rewrite all of the existing trace config code, so that things stop
> > being hairy. I believe that's what Primiano is doing for the next 6 months.
>
> I think making MemoryDumpConfig an opaque object before its structure settles
> would unblock you:
>
> {
> "id": "MemoryDumpConfig",
> "type": "object",
> "description": "Memory config."
> },
>
> That would be represented as a base::DictionaryValue in code that you would be
> able to parse manually.
Done. PTAL.
On 2016/05/02 17:42:17, Maria wrote:
> On 2016/05/02 17:26:28, pfeldman wrote:
> > > Pavel,
> > >
> > > I'd appreciate if you can provide some actionable next steps. I am happy
to:
> > > 1) Remove API documentation from devtools
> > > 2) Make specific API changes that are reasonable in scope
> > >
> > > I cannot rewrite all of the existing trace config code, so that things
stop
> > > being hairy. I believe that's what Primiano is doing for the next 6
months.
> >
> > I think making MemoryDumpConfig an opaque object before its structure
settles
> > would unblock you:
> >
> > {
> > "id": "MemoryDumpConfig",
> > "type": "object",
> > "description": "Memory config."
> > },
> >
> > That would be represented as a base::DictionaryValue in code that you would
be
> > able to parse manually.
>
> Done. PTAL.
ping. Pavel and Dmitry I am still waiting for your approvals of protocol.json
and the unittest respectively.
protocol lgtm
The CQ bit was checked by mariakhomenko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1911643002/#ps200001 (title: "Remove memory dump config details from devtools")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911643002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911643002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by mariakhomenko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, haraken@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1911643002/#ps260001 (title: "Whitespace change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911643002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911643002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add configurable limit to allocations in heap profiler. Adds a new trace config parameter, min_allocation_size_bytes, which determines the cutoff for allocation sizes below which the traces for that allocation are excluded from the trace file. Makes the default cutoff to be 1024 bytes. BUG=605298 ========== to ========== Add configurable limit to allocations in heap profiler. Adds a new trace config parameter, min_allocation_size_bytes, which determines the cutoff for allocation sizes below which the traces for that allocation are excluded from the trace file. Makes the default cutoff to be 1024 bytes. BUG=605298 Committed: https://crrev.com/3b1e75900d721b57351754318af8bcfaa7815744 Cr-Commit-Position: refs/heads/master@{#391773} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/3b1e75900d721b57351754318af8bcfaa7815744 Cr-Commit-Position: refs/heads/master@{#391773} |
