Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2845633002:
memory-infra: Remove is_enabled_ from MDM (Closed)
Descriptionmemory-infra: Remove is_enabled_ from MDM
This is one step the eventual refactor to remove Enable/Disable.
BUG=705564
Review-Url: https://codereview.chromium.org/2845633002
Cr-Commit-Position: refs/heads/master@{#470340}
Committed: https://chromium.googlesource.com/chromium/src/+/49ce146674cbe4be5d7e1b53d270d229830a64ce
Patch Set 1 #
Total comments: 15
Patch Set 2 : rebase #Patch Set 3 : rebase again #Patch Set 4 : address comments #
Total comments: 14
Patch Set 5 : rebase #Patch Set 6 : address comments #Patch Set 7 : rebase #Patch Set 8 : fixrebase #Patch Set 9 : update to fix tests broken by rebase #Patch Set 10 : add unit test #Patch Set 11 : add not reached #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by hjd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hjd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hjd@chromium.org changed reviewers: + primiano@chromium.org, ssid@chromium.org
So this does pass all the tests... I'm not convinced it's right though
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Makes sense to me. After ssid cleanup (see CL linked in comment below) I think this becomes way smoother and less awkward. ssid, comments? Am I missing something? https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:396: tracing_observer_->IsMemoryInfraTracingEnabled()) { ssid is ripping out this code for you in https://codereview.chromium.org/2844373002/ which I just LGTM'd Which I guess is what caused the most awkwardness https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:437: if (!IsDumpModeAllowed(level_of_detail)) { I think we should have this check only in the ::Finalize method, before calling the TracingObserver->MaybeAddToTrace() which kinda feels like a dejavu because I though we did that already? (or maybe I start confusing CL I reviewed vs CLs that exist only in my mind) https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_observer.h:35: bool IsMemoryInfraTracingEnabled(); I think we don't need this, given the comment above
Still thinking if we have more cases. ptal at my comments. Thanks for doing this! https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:892: subtle::NoBarrier_Store(&is_enabled_, 0); Yay we don't need this anymore :D https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:396: tracing_observer_->IsMemoryInfraTracingEnabled()) { On 2017/04/28 15:58:17, Primiano Tucci wrote: > ssid is ripping out this code for you in > https://codereview.chromium.org/2844373002/ which I just LGTM'd > Which I guess is what caused the most awkwardness Sorry i did not review this cl because of the mentioned cl. Please rebase. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:437: if (!IsDumpModeAllowed(level_of_detail)) { On 2017/04/28 15:58:17, Primiano Tucci wrote: > I think we should have this check only in the ::Finalize method, before calling > the TracingObserver->MaybeAddToTrace() > > which kinda feels like a dejavu because I though we did that already? (or maybe > I start confusing CL I reviewed vs CLs that exist only in my mind) No this used to exist here and not even trigger wrong dumps. Makes perfect sense to just move this to just before MaybeAddToTrace call. Also we need a failure condition there to run the callback if dump mode is not allowed? I think the case in Finalize() should be: If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) { callback.Run(false /* failure */); } else { MaybeAddToTrace(); } Do we still need to optimize the case where some component r4equests a detailed dump when only background dumps are allowed in the tracing session, then we just don't trigger the dumps instead early out with failure? I think it'd be a good idea to have a check here too. So, with that logic the option would be: in RequestGlobalDump(): If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) { callback.Run(false) return; } in Finalize(): CHECK(IsDumpModeAllowed() || dump_type == SUMMARY_ONLY); I am fine with either case. I think having the optimization is better. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:527: We should have a condition here that says: if (level_of_detail === DETAILED && heap_profiling_enabled_ && !session_state_) { callback.Run(false); return; } If we have heap profiling enabled and no session then PMD will later expect a session state while creating heap dump. So before creating PMDAS we need to check if session state is null. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:536: session_state_->IsDumpModeAllowed(args.level_of_detail)); If we are going with the first option of first creating dump and then deciding about adding to trace this check should be removed since it will crash if someone requests a detailed dump. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:875: bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) { Maybe rename to DoesSessionAllowDumpMode? Since without having a session any dump mode is allowed. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_observer.h:35: bool IsMemoryInfraTracingEnabled(); On 2017/04/28 15:58:17, Primiano Tucci wrote: > I think we don't need this, given the comment above Yes!! :)
https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:892: subtle::NoBarrier_Store(&is_enabled_, 0); On 2017/04/28 18:35:04, ssid wrote: > Yay we don't need this anymore :D :D https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:437: if (!IsDumpModeAllowed(level_of_detail)) { On 2017/04/28 18:35:04, ssid wrote: > On 2017/04/28 15:58:17, Primiano Tucci wrote: > > I think we should have this check only in the ::Finalize method, before > calling > > the TracingObserver->MaybeAddToTrace() > > > > which kinda feels like a dejavu because I though we did that already? (or > maybe > > I start confusing CL I reviewed vs CLs that exist only in my mind) > > No this used to exist here and not even trigger wrong dumps. Makes perfect sense > to just move this to just before MaybeAddToTrace call. > Also we need a failure condition there to run the callback if dump mode is not > allowed? I think the case in Finalize() should be: > > If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) { > callback.Run(false /* failure */); > } else { > MaybeAddToTrace(); > } > > > Do we still need to optimize the case where some component r4equests a detailed > dump when only background dumps are allowed in the tracing session, then we just > don't trigger the dumps instead early out with failure? I think it'd be a good > idea to have a check here too. > > So, with that logic the option would be: > > in RequestGlobalDump(): > If (!IsDumpModeAllowed() && dump_type != SUMMARY_ONLY) { > callback.Run(false) > return; > } > > in Finalize(): > CHECK(IsDumpModeAllowed() || dump_type == SUMMARY_ONLY); > > > I am fine with either case. I think having the optimization is better. Done. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:527: On 2017/04/28 18:35:04, ssid wrote: > We should have a condition here that says: > > if (level_of_detail === DETAILED && heap_profiling_enabled_ && !session_state_) > { > callback.Run(false); > return; > } > > If we have heap profiling enabled and no session then PMD will later expect a > session state while creating heap dump. > So before creating PMDAS we need to check if session state is null. Done. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:536: session_state_->IsDumpModeAllowed(args.level_of_detail)); On 2017/04/28 18:35:04, ssid wrote: > If we are going with the first option of first creating dump and then deciding > about adding to trace this check should be removed since it will crash if > someone requests a detailed dump. Acknowledged. https://codereview.chromium.org/2845633002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:875: bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) { On 2017/04/28 18:35:04, ssid wrote: > Maybe rename to DoesSessionAllowDumpMode? > Since without having a session any dump mode is allowed. Done.
minor comments, mostly looks good. Feel free to edit the comment suggestions as you like. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:414: DCHECK(dump_type != SUMMARY_ONLY || level_of_detail == BACKGROUND); https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:497: if (args.level_of_detail == MemoryDumpLevelOfDetail::DETAILED && Comment here saying: MDM could have been disabled before reaching to this point. Since PMD will expect session state if heap profiling is enabled, return without creating the dump. Thinking more on this I realized in future we could have heap profiling with background mode for slow reports. So, a better condition would be args.dump_type != SUMMARY_ONLY. Sorry for the back and forth. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:116: // filters and categories specified when enabling tracing. A SUMMARY_ONLY dump can be requested at any time after initialization and other type of dumps can be requested only when MDM is enabled. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:129: // Prepare MemoryDumpManager for RequestGlobalMemoryDump calls. Prepare MemoryDumpManager for RequestGlobalDump calls for tracing related modes (non-SUMMARY_ONLY) https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:130: // Starts the MemoryDumpManager thread. This comment is probably stale? https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:131: // Also uses the given config to initialize the peak detector, Also uses - > Initializes the peak... https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:135: // Tearsdown the MemoryDumpManager thread and various other state set up by remove the part about Thread.
Actually lgtm, if primiano can take a pass.
updated, thanks! https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:414: On 2017/05/04 22:00:22, ssid wrote: > DCHECK(dump_type != SUMMARY_ONLY || level_of_detail == BACKGROUND); Done. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:497: if (args.level_of_detail == MemoryDumpLevelOfDetail::DETAILED && On 2017/05/04 22:00:22, ssid wrote: > Comment here saying: > MDM could have been disabled before reaching to this point. Since PMD will > expect session state if heap profiling is enabled, return without creating the > dump. > > Thinking more on this I realized in future we could have heap profiling with > background mode for slow reports. So, a better condition would be args.dump_type > != SUMMARY_ONLY. Sorry for the back and forth. Done. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:116: // filters and categories specified when enabling tracing. On 2017/05/04 22:00:22, ssid wrote: > A SUMMARY_ONLY dump can be requested at any time after initialization and other > type of dumps can be requested only when MDM is enabled. Done. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:129: // Prepare MemoryDumpManager for RequestGlobalMemoryDump calls. On 2017/05/04 22:00:22, ssid wrote: > Prepare MemoryDumpManager for RequestGlobalDump calls for tracing related modes > (non-SUMMARY_ONLY) Done. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:130: // Starts the MemoryDumpManager thread. On 2017/05/04 22:00:22, ssid wrote: > This comment is probably stale? Done, thanks! https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:131: // Also uses the given config to initialize the peak detector, On 2017/05/04 22:00:22, ssid wrote: > Also uses - > Initializes the peak... Done. https://codereview.chromium.org/2845633002/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:135: // Tearsdown the MemoryDumpManager thread and various other state set up by On 2017/05/04 22:00:22, ssid wrote: > remove the part about Thread. Done.
great, lgtm thanks!
The CQ bit was checked by hjd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by hjd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2845633002/#ps200001 (title: "add not reached")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494334902541080,
"parent_rev": "4645ee94b0d729532c32eb504e7e1fe400206548", "commit_rev":
"49ce146674cbe4be5d7e1b53d270d229830a64ce"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Remove is_enabled_ from MDM This is one step the eventual refactor to remove Enable/Disable. BUG=705564 ========== to ========== memory-infra: Remove is_enabled_ from MDM This is one step the eventual refactor to remove Enable/Disable. BUG=705564 Review-Url: https://codereview.chromium.org/2845633002 Cr-Commit-Position: refs/heads/master@{#470340} Committed: https://chromium.googlesource.com/chromium/src/+/49ce146674cbe4be5d7e1b53d270... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/49ce146674cbe4be5d7e1b53d270... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
