memory-infra: Move all clients of RequestGlobalDump to the memory-infra service |
|
Issue description[low prio right now, but need a tracking bug for a CL] Today we have this weird situation where our public RequestGlobalDump() API is exposed in base::MemoryDumpManager (used by the current clients: devtools, tracing) and in the service. Base::MDM has a simplified interface (just returns a boolean |success|) while the service is able to return the summarized dump with CMM and whatnot. In the long term the service should be the only one having the API and everything (devtools, tracing) should interface with that. Until then we have to maintain these weird hops where base::MDM calls indirectly (via an injected function pointer) into the service, and then the service bounces back the callback to MDM. Even worse, since we support requesting dumps from any process, we might do this bouncing twice, one in the renderer and one in the browser process. This is just extremely hard to follow, although keeps me strongly employed. After the first MVP of memory UMA we should make myself less employable and remove these acrobatic jumps.
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac66f89e4be521caa45a972b20e7890992901469 commit ac66f89e4be521caa45a972b20e7890992901469 Author: Sam McNally <sammc@chromium.org> Date: Thu Jun 08 03:45:13 2017 Revert "memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest" This reverts commit 16e1788871253de5241941eaeb0a5fdb34b3d21f. Reason for revert: MemoryDumpManagerTest.TestBackgroundTracingSetup fails under LSAN: https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20ASan%20LSan%20Tests%20%281%29/36339 Original change's description: > memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest > > This CL changes the memory-infra unittests to not depend on > the RequestGlobalDump() API. From a logical viewpoint, > the RequestGlobalDump() is not needed for an in-process-only > test like MemoryDumpManagerTest. > From a more practical viewpoint, this refactoring is required > in preparation of the next CLs that will remove > MemoryDumpManager::RequestGlobalDump() method and move all the > clients to directly use the service. > > This CL doesn't cause any behavior change in memory-infra. > > BUG=720352 > TBR=thakis > > Change-Id: I82ee1bf94dab55798be727f0ae70a1fede6634ca > Reviewed-on: https://chromium-review.googlesource.com/525534 > Reviewed-by: Primiano Tucci <primiano@chromium.org> > Reviewed-by: siddhartha sivakumar <ssid@chromium.org> > Reviewed-by: Helen Li <xunjieli@chromium.org> > Reviewed-by: Hector Dearman <hjd@chromium.org> > Commit-Queue: siddhartha sivakumar <ssid@chromium.org> > Cr-Commit-Position: refs/heads/master@{#477851} TBR=thakis@chromium.org,primiano@chromium.org,hjd@chromium.org,xunjieli@chromium.org,ssid@chromium.org No-Presubmit: true No-Tree-Checks: true No-Try: true BUG=720352 Change-Id: I7088bd3549c3a8a733d4b87226593209bd40c42e Reviewed-on: https://chromium-review.googlesource.com/528032 Reviewed-by: Sam McNally <sammc@chromium.org> Commit-Queue: Sam McNally <sammc@chromium.org> Cr-Commit-Position: refs/heads/master@{#477875} [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/base/BUILD.gn [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/base/trace_event/memory_dump_manager.h [delete] https://crrev.com/08484a895215d9d328082c98189832c52b3a505b/base/trace_event/memory_dump_manager_test_utils.h [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/net/url_request/url_request_quic_perftest.cc [modify] https://crrev.com/ac66f89e4be521caa45a972b20e7890992901469/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
,
Jun 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31b7833922c241a5495b1c42af67ca9503cdb65d commit 31b7833922c241a5495b1c42af67ca9503cdb65d Author: John Budorick <jbudorick@chromium.org> Date: Thu Jun 08 22:47:13 2017 Revert "Reland: memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest" This reverts commit 05755b0fb4e7437150ab60375739e2cf8b0b3aed. Reason for revert: Failing frequently on linux_chromium_asan_rel_ng on the CQ. Has failed once so far on CI: https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%281%29/builds/36356 Original change's description: > Reland: memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest > > This reverts commit ac66f89e4be521caa45a972b20e7890992901469. > > Reason for reland: Fixed race in the test > > Original change's description: > > Revert "memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest" > > > > This reverts commit 16e1788871253de5241941eaeb0a5fdb34b3d21f. > > > > Reason for revert: MemoryDumpManagerTest.TestBackgroundTracingSetup fails under LSAN: https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20ASan%20LSan%20Tests%20%281%29/36339 > > > > Original change's description: > > > memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest > > > > > > This CL changes the memory-infra unittests to not depend on > > > the RequestGlobalDump() API. From a logical viewpoint, > > > the RequestGlobalDump() is not needed for an in-process-only > > > test like MemoryDumpManagerTest. > > > From a more practical viewpoint, this refactoring is required > > > in preparation of the next CLs that will remove > > > MemoryDumpManager::RequestGlobalDump() method and move all the > > > clients to directly use the service. > > > > > > This CL doesn't cause any behavior change in memory-infra. > > > > > > BUG=720352 > > > TBR=thakis > > > > > > Change-Id: I82ee1bf94dab55798be727f0ae70a1fede6634ca > > > Reviewed-on: https://chromium-review.googlesource.com/525534 > > > Reviewed-by: Primiano Tucci <primiano@chromium.org> > > > Reviewed-by: siddhartha sivakumar <ssid@chromium.org> > > > Reviewed-by: Helen Li <xunjieli@chromium.org> > > > Reviewed-by: Hector Dearman <hjd@chromium.org> > > > Commit-Queue: siddhartha sivakumar <ssid@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#477851} > > > > TBR=thakis@chromium.org,primiano@chromium.org,hjd@chromium.org,xunjieli@chromium.org,ssid@chromium.org > > No-Presubmit: true > > No-Tree-Checks: true > > No-Try: true > > BUG=720352 > > > > Change-Id: I7088bd3549c3a8a733d4b87226593209bd40c42e > > Reviewed-on: https://chromium-review.googlesource.com/528032 > > Reviewed-by: Sam McNally <sammc@chromium.org> > > Commit-Queue: Sam McNally <sammc@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#477875} > > TBR=thakis@chromium.org,jam@chromium.org,primiano@chromium.org,fmeawad@chromium.org,hjd@chromium.org,sammc@chromium.org,xunjieli@chromium.org,ssid@chromium.org,chromium-reviews@chromium.org > > Change-Id: Ic6439b97024d65d19012133171249dddd2de8b88 > Reviewed-on: https://chromium-review.googlesource.com/528098 > Commit-Queue: Primiano Tucci <primiano@chromium.org> > Reviewed-by: Primiano Tucci <primiano@chromium.org> > Cr-Commit-Position: refs/heads/master@{#477988} TBR=thakis@chromium.org,jam@chromium.org,primiano@chromium.org,fmeawad@chromium.org,hjd@chromium.org,sammc@chromium.org,xunjieli@chromium.org,ssid@chromium.org,chromium-reviews@chromium.org No-Presubmit: true No-Tree-Checks: true No-Try: true Change-Id: I0e9f5a6f930e096406ac0d46ac310ef2fae99370 Reviewed-on: https://chromium-review.googlesource.com/528513 Reviewed-by: John Budorick <jbudorick@chromium.org> Commit-Queue: John Budorick <jbudorick@chromium.org> Cr-Commit-Position: refs/heads/master@{#478109} [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/base/BUILD.gn [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/base/trace_event/memory_dump_manager.h [delete] https://crrev.com/875542f160c0e992786f35e6ef48bbaa065b17bb/base/trace_event/memory_dump_manager_test_utils.h [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/net/url_request/url_request_quic_perftest.cc [modify] https://crrev.com/31b7833922c241a5495b1c42af67ca9503cdb65d/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
,
Jun 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3391c723f302ea1b889c59dab2cb8dd160c859c9 commit 3391c723f302ea1b889c59dab2cb8dd160c859c9 Author: Primiano Tucci <primiano@chromium.org> Date: Mon Jun 12 15:03:59 2017 Reland(2): memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest This reverts commit 31b7833922c241a5495b1c42af67ca9503cdb65d. Reason for reland: Fixed TestBackgroundTracingSetup flakiness, this time for real. In the last reland I forgot to git stash pop the actual fix and ended up just relanding the original CL as-is. Original change's description: > Revert "Reland: memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest" > > This reverts commit 05755b0fb4e7437150ab60375739e2cf8b0b3aed. > > Reason for revert: Failing frequently on linux_chromium_asan_rel_ng on the CQ. Has failed once so far on CI: https://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%281%29/builds/36356 > > Original change's description: > > Reland: memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest > > > > This reverts commit ac66f89e4be521caa45a972b20e7890992901469. > > > > Reason for reland: Fixed race in the test > > > > Original change's description: > > > Revert "memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest" > > > > > > This reverts commit 16e1788871253de5241941eaeb0a5fdb34b3d21f. > > > > > > Reason for revert: MemoryDumpManagerTest.TestBackgroundTracingSetup fails under LSAN: https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20ASan%20LSan%20Tests%20%281%29/36339 > > > > > > Original change's description: > > > > memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest > > > > > > > > This CL changes the memory-infra unittests to not depend on > > > > the RequestGlobalDump() API. From a logical viewpoint, > > > > the RequestGlobalDump() is not needed for an in-process-only > > > > test like MemoryDumpManagerTest. > > > > From a more practical viewpoint, this refactoring is required > > > > in preparation of the next CLs that will remove > > > > MemoryDumpManager::RequestGlobalDump() method and move all the > > > > clients to directly use the service. > > > > > > > > This CL doesn't cause any behavior change in memory-infra. > > > > > > > > BUG=720352 > > > > TBR=thakis > > > > > > > > Change-Id: I82ee1bf94dab55798be727f0ae70a1fede6634ca > > > > Reviewed-on: https://chromium-review.googlesource.com/525534 > > > > Reviewed-by: Primiano Tucci <primiano@chromium.org> > > > > Reviewed-by: siddhartha sivakumar <ssid@chromium.org> > > > > Reviewed-by: Helen Li <xunjieli@chromium.org> > > > > Reviewed-by: Hector Dearman <hjd@chromium.org> > > > > Commit-Queue: siddhartha sivakumar <ssid@chromium.org> > > > > Cr-Commit-Position: refs/heads/master@{#477851} > > > > > > TBR=thakis@chromium.org,primiano@chromium.org,hjd@chromium.org,xunjieli@chromium.org,ssid@chromium.org > > > No-Presubmit: true > > > No-Tree-Checks: true > > > No-Try: true > > > BUG=720352 > > > > > > Change-Id: I7088bd3549c3a8a733d4b87226593209bd40c42e > > > Reviewed-on: https://chromium-review.googlesource.com/528032 > > > Reviewed-by: Sam McNally <sammc@chromium.org> > > > Commit-Queue: Sam McNally <sammc@chromium.org> > > > Cr-Commit-Position: refs/heads/master@{#477875} > > > > TBR=thakis@chromium.org,jam@chromium.org,primiano@chromium.org,fmeawad@chromium.org,hjd@chromium.org,sammc@chromium.org,xunjieli@chromium.org,ssid@chromium.org,chromium-reviews@chromium.org > > > > Change-Id: Ic6439b97024d65d19012133171249dddd2de8b88 > > Reviewed-on: https://chromium-review.googlesource.com/528098 > > Commit-Queue: Primiano Tucci <primiano@chromium.org> > > Reviewed-by: Primiano Tucci <primiano@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#477988} > > TBR=thakis@chromium.org,jam@chromium.org,primiano@chromium.org,fmeawad@chromium.org,hjd@chromium.org,sammc@chromium.org,xunjieli@chromium.org,ssid@chromium.org,chromium-reviews@chromium.org > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > > Change-Id: I0e9f5a6f930e096406ac0d46ac310ef2fae99370 > Reviewed-on: https://chromium-review.googlesource.com/528513 > Reviewed-by: John Budorick <jbudorick@chromium.org> > Commit-Queue: John Budorick <jbudorick@chromium.org> > Cr-Commit-Position: refs/heads/master@{#478109} TBR=thakis@chromium.org,jam@chromium.org,primiano@chromium.org,fmeawad@chromium.org,hjd@chromium.org,sammc@chromium.org,xunjieli@chromium.org,ssid@chromium.org,chromium-reviews@chromium.org,jbudorick@chromium.org Change-Id: I7493014373569281e84049cb6fdca987fc9ee83e Reviewed-on: https://chromium-review.googlesource.com/530311 Commit-Queue: Primiano Tucci <primiano@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#478623} [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/base/BUILD.gn [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/base/trace_event/memory_dump_manager.h [add] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/base/trace_event/memory_dump_manager_test_utils.h [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/net/url_request/url_request_quic_perftest.cc [modify] https://crrev.com/3391c723f302ea1b889c59dab2cb8dd160c859c9/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5640bf194652e93b1b8562f588ae3a8f0e672148 commit 5640bf194652e93b1b8562f588ae3a8f0e672148 Author: Primiano Tucci <primiano@chromium.org> Date: Thu Jun 22 21:55:49 2017 memory-infra: get rid of MemoryDumpManager::RequestGlobalDump method. This CL gets rid of the MemoryDumpManager::RequestGlobalDump(), and moves the existing code to use the service. This allows to cleanup all the odd callback proxy required to match the base vs mojo callbacks. Also, this moves the dump id generation responsibility to the service, which is now the only one dealing with global dumps. At this point the only knowledge left in base::MemoryDumpManager about "global" dumps is only the |request_dump_function|, which is required to keep the periodic dump scheduler and peak detector working. This can be removed once those two are moved to the service. BUG=720352 Change-Id: I25c22157234e1e627c5aeafa189df01944354e7d Reviewed-on: https://chromium-review.googlesource.com/536952 Commit-Queue: Primiano Tucci <primiano@chromium.org> Reviewed-by: siddhartha sivakumar <ssid@chromium.org> Reviewed-by: Hector Dearman <hjd@chromium.org> Cr-Commit-Position: refs/heads/master@{#481678} [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/base/trace_event/memory_dump_manager.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/base/trace_event/memory_dump_manager.h [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/base/trace_event/memory_dump_manager_test_utils.h [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/base/trace_event/memory_dump_request_args.h [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/content/browser/tracing/memory_tracing_browsertest.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/memory_instrumentation/coordinator_impl.h [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc [modify] https://crrev.com/5640bf194652e93b1b8562f588ae3a8f0e672148/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h |
|
►
Sign in to add a comment |
|
Comment 1 by bugdroid1@chromium.org
, Jun 8 2017