memory-infra: clean up level-of-detail vs dump-type |
|||
Issue description
This part of memory-infra grew up "organically" and now is objectively an utter mess.
This bug proposes a long-term solution as well as a gradual approach to get there.
Current state
-------------
DumpType:
PERIODIC_INTERVAL, // Dumping memory at periodic intervals.
EXPLICITLY_TRIGGERED, // Non maskable dump request.
PEAK_MEMORY_USAGE, // Dumping memory at detected peak total memory usage.
SUMMARY_ONLY, // Calculate just the summary & don't add to the trace.
VM_REGIONS_ONLY,
LevelOfDetail:
BACKGROUND, // For UMA/UKM/SlowReports
LIGHT, // For tracing mostly, really unused these days
DETAILED, // For tracing and telemetry
Problems:
---------
## Problem 1:
Some of the dump types are really, conceptually, level of details. Specifically:
- VM_REGIONS_ONLY: This is for the heap profiler and is just my fault for having put that under DUmpType instead of LevelOfDetail without thinking too much.
- SUMMARY_ONLY: This is more subtle. This is essentially a BACKGROUND dump with a more restricted whitelist. This is temporary and just to support the 4-5 MDPs today we have enable for UMA/UKM and won't be needed once we use the full set of BACKGROUND MDP (the same of slow reports) for UMA/UKM.
## Problem 2:
The remaining DumpTypes have different semantics attached to them which are not explicit and subtle. Specifically:
- PEAK_MEMORY_USAGE: Is temporarily unused as we put the peak detector work on hold. I think what we want to communicate here is really just the same of PERIODIC_INTERVAL below
- PERIODIC_INTERVAL: the semantic we really want here is about the queueing strategy. If another PERIODIC_INTERVAL dump with the same level of detail is queued, we shouldn't not queue another one to avoid building up a backlog. Also this is really only for the RequestDumpAndAddToTrace. We don't have any use case today for the other RequestDump(AndConsumeItFromWithinChrome). In other words RequestDump(AndConsumeItFromWithinChrome) is always EXPLICITLY_TRIGGERED.
- EXPLICITLY_TRIGGERED: what this wants to say is: "just do a dump, no question asked, no throttling / backlog prevention"
Intended final state:
---------------------
IMHO a good final state is assigning a strong semantic to both, specifically:
LevelOfDetail: should purely be used to condition, as the name suggest, how rich are the dumps.
QueueingStrategy: should purely be used to describe what happens in case of queueing.
We should end up in a state where
- LevelOfDetails becomes: {BACKGROUND, LIGHT, DETAILED, VM_REGIONS_ONLY_FOR_HEAP_PROFILER}
- DumpType is renamed to QueueingStrategy { SKIP_IF_ALREADY_QUEUED, NEVER_SKIP}
- memory_instrumentation::RequestGlobalDump() takes LevelOfDetail as only argument.
- memory_instrumentation::RequestGlobalDumpAndAddToTrace() takes both LevelOfDetail and QueueingStrategy
How to get there incrementally:
-------------------------------
1) Move VM_REGIONS_ONLY to LevelOfDetail, this should be quite straightforward.
2) Move the peak detector code to just use PERIODIC_INTERVAL, should work anyways for the moment, and kill PEAK_MEMORY_USAGE
3) Finish the work on Issue 768373 (full MDPs available from chrome) so that we don't need SUMMARY_ONLY and that becomes just BACKGROUND.
4) At this point we are left with DumpType being only {PERIODIC_INTERVAL, EXPLICITLY_TRIGGERED} and it can be renamed to QueueingStrategy above.
Like in "Home Alone" (the movie) I feel I missed some other subtle semantic attached to the combinations of these two. If you can tell which one speak up in this bug.
Also please mark related bugs as dependencies of this one so we can track the work here.
,
Oct 24 2017
,
Oct 24 2017
Re. point 2): We cannot remove PEAK_MEMORY_USAGE because it is used by MemoryDumpManager as a trigger type for tracing. Until we transition that code to not be in base, we cannot remove PEAK_MEMORY_USAGE.
,
Oct 24 2017
+primiano Thanks for looking into this. The final state sounds good.
,
Oct 24 2017
I think we need to differentiate periodic and peak type in trace config. Also we need to keep around "trigger_type" in trace config for legacy reason? We also need to update telemetry bots and keep around the old config till these changes reach stable. (https://chromium.googlesource.com/chromium/src/+/d585a0ee966bc03d8a66eecbeaf12b589944bdf5/base/trace_event/trace_config.cc#420)
,
Oct 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788 commit 376991ca05d1dfc07ccc8b3d90e0497a4d3e3788 Author: Lalit Maganti <lalitm@chromium.org> Date: Tue Oct 24 18:59:28 2017 memory-infra: move VM_REGIONS_ONLY from a type to level of detail VM_REGIONS_ONLY never made sense as a type. It was talking about what needed to be returned which is captured by level of detail rather than the type. With the big refactoring of the whole memory dump arguments incoming, moving this clears the path for working on more complex issues in the system. Bug: 775034 Bug: 777820 Change-Id: If2ada9c4dd3cbd25a1c5b49dec2ec3e68f9c4f95 Reviewed-on: https://chromium-review.googlesource.com/735323 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Commit-Queue: Lalit Maganti <lalitm@chromium.org> Cr-Commit-Position: refs/heads/master@{#511221} [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/base/trace_event/memory_dump_request_args.cc [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/base/trace_event/memory_dump_request_args.h [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/base/trace_event/memory_dump_scheduler.cc [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/services/resource_coordinator/memory_instrumentation/queued_request.h [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation_struct_traits.cc [modify] https://crrev.com/376991ca05d1dfc07ccc8b3d90e0497a4d3e3788/services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom
,
Oct 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/33684a9efdc271b9804e0c6595d26e1985ac65ff commit 33684a9efdc271b9804e0c6595d26e1985ac65ff Author: Lalit Maganti <lalitm@chromium.org> Date: Wed Oct 25 23:38:57 2017 memory-infra: move SUMMARY_ONLY from type to level of detail As with VM_REGIONS_ONLY, SUMMARY_ONLY never belonged in the type enum as its meaning refers how much information should be returned to the caller of the memory instrumentation service. Move the enum member and change all relevant references. Eventually, we plan to remove this member all together as its purpose will be subsumed by one of the other levels of detail but for now this is a good stepping stone. Bug: 775041 Bug: 777820 Change-Id: I43504be0cd2f864b38dfd607b7d08ada156ed7ca Reviewed-on: https://chromium-review.googlesource.com/735700 Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Reviewed-by: Siddhartha S <ssid@chromium.org> Commit-Queue: Lalit Maganti <lalitm@chromium.org> Cr-Commit-Position: refs/heads/master@{#511645} [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/base/trace_event/memory_dump_request_args.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/base/trace_event/memory_dump_request_args.h [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/base/trace_event/memory_dump_scheduler.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/chrome/browser/metrics/process_memory_metrics_emitter.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/memory_instrumentation/queued_request.h [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation_struct_traits.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/public/cpp/memory_instrumentation/struct_traits_unittest.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc [modify] https://crrev.com/33684a9efdc271b9804e0c6595d26e1985ac65ff/services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom
,
Oct 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98e8b7f9910e484189d4806ee1d5e456cb31701c commit 98e8b7f9910e484189d4806ee1d5e456cb31701c Author: Lalit Maganti <lalitm@chromium.org> Date: Thu Oct 26 09:46:29 2017 Revert "memory-infra: move SUMMARY_ONLY from type to level of detail" This reverts commit 33684a9efdc271b9804e0c6595d26e1985ac65ff. Reason for revert: A lot of code is conditional on BACKGROUND being the level of detail and so there are significant problems if we don't revert this including the crasher motivating this bug but also issues with UMA and UKM etc. Bug: 778560 Original change's description: > memory-infra: move SUMMARY_ONLY from type to level of detail > > As with VM_REGIONS_ONLY, SUMMARY_ONLY never belonged in the type enum as > its meaning refers how much information should be returned to the caller > of the memory instrumentation service. > > Move the enum member and change all relevant references. Eventually, we > plan to remove this member all together as its purpose will be subsumed > by one of the other levels of detail but for now this is a good stepping > stone. > > Bug: 775041 > Bug: 777820 > Change-Id: I43504be0cd2f864b38dfd607b7d08ada156ed7ca > Reviewed-on: https://chromium-review.googlesource.com/735700 > Reviewed-by: Robert Kaplow <rkaplow@chromium.org> > Reviewed-by: Daniel Cheng <dcheng@chromium.org> > Reviewed-by: Primiano Tucci <primiano@chromium.org> > Reviewed-by: Siddhartha S <ssid@chromium.org> > Commit-Queue: Lalit Maganti <lalitm@chromium.org> > Cr-Commit-Position: refs/heads/master@{#511645} TBR=dcheng@chromium.org,primiano@chromium.org,rkaplow@chromium.org,hjd@chromium.org,ssid@chromium.org,lalitm@chromium.org Change-Id: I5a765e551eb1d9cac289a13eb9e373c72ddd78e4 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 775041, 777820 Reviewed-on: https://chromium-review.googlesource.com/738039 Reviewed-by: Primiano Tucci <primiano@chromium.org> Commit-Queue: Lalit Maganti <lalitm@chromium.org> Cr-Commit-Position: refs/heads/master@{#511774} [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/base/trace_event/memory_dump_request_args.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/base/trace_event/memory_dump_request_args.h [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/base/trace_event/memory_dump_scheduler.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/chrome/browser/metrics/process_memory_metrics_emitter.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/memory_instrumentation/queued_request.h [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation_struct_traits.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/public/cpp/memory_instrumentation/struct_traits_unittest.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc [modify] https://crrev.com/98e8b7f9910e484189d4806ee1d5e456cb31701c/services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom
,
Oct 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae67c486ce91951a5ef5e38b78ebdf567fac54a3 commit ae67c486ce91951a5ef5e38b78ebdf567fac54a3 Author: Hector Dearman <hjd@google.com> Date: Thu Oct 26 18:20:33 2017 memory-infra: Split global from local RequestDumpArgs memory_instrumentation::RequestGlobalMemoryDump api should not contain guid argument. Bug: 775037 Bug: 777820 Change-Id: I057065eaf1ac6ed6a295343413d7236463c6ac55 Reviewed-on: https://chromium-review.googlesource.com/723461 Commit-Queue: Hector Dearman <hjd@chromium.org> Reviewed-by: Siddhartha S <ssid@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#511885} [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/base/trace_event/memory_dump_manager.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/base/trace_event/memory_dump_manager_test_utils.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/base/trace_event/memory_dump_request_args.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/chrome/browser/metrics/process_memory_metrics_emitter.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/memory_instrumentation/coordinator_impl.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.typemap [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation_struct_traits.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation_struct_traits.h [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc [modify] https://crrev.com/ae67c486ce91951a5ef5e38b78ebdf567fac54a3/services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom |
|||
►
Sign in to add a comment |
|||
Comment 1 by lalitm@chromium.org
, Oct 24 2017