Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883693002:
[Memory-UMA] Implement basic working prototype. (Closed)
|
Created:
3 years, 7 months ago by erikchen Modified:
3 years, 7 months ago CC:
chromium-reviews, chrome-grc-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[reland 1] [Memory-UMA] Implement basic working prototype.
The first attempt to land this CL had test failures on ChromeOS for the newly
added tests. The tests have been updated to also work on ChromeOS.
> This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/.
>
> This CL includes the following changes:
> * Fix histogram emissions to not be off by a factor of 2 ** 20.
> * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the
> scope of end-to-end tests to sanity check UMA emissions.
> * Change the location where the object that backs the memory instrumentation
> interface is created, as it now requires Mojo to be initialized.
> * The implementation of the memory instrumentation interface now requires
> ServiceManagerListener, so add that requirement to
> content_browser_manifest.json.
> * Create a ProcessMap class that maps service identifiers to pids.
> * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps.
>
> BUG=703184
> TBR=rkaplow@chromium.org
>
> Review-Url: https://codereview.chromium.org/2883693002
> Cr-Commit-Position: refs/heads/master@{#471964}
> Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55898f5df1b4d2
BUG=703184
Review-Url: https://codereview.chromium.org/2883693002
Cr-Commit-Position: refs/heads/master@{#472659}
Committed: https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc1c72f683c16d
Patch Set 1 : Cloned from https://codereview.chromium.org/2875673003/#ps80001. #Patch Set 2 : tests are still wip #Patch Set 3 : Tests working. #
Total comments: 20
Patch Set 4 : comemnts from dcheng. #Patch Set 5 : clean up #Patch Set 6 : disable test on ASAN. #Patch Set 7 : Respecify dependent CL, which had somehow gotten lost. #
Total comments: 28
Patch Set 8 : Comments from primiano. #Patch Set 9 : fix macro. #Patch Set 10 : Disable test on ASAN. #Patch Set 11 : Fix tests for ChromeOS. #Patch Set 12 : Fix tests #Patch Set 13 : Fix test. #Messages
Total messages: 83 (51 generated)
Description was changed from ========== [Memory-UMA] Add ProcessMap, an Identity to Pid map This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. BUG=703184 ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 ==========
The CQ bit was checked by erikchen@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...
erikchen@chromium.org changed reviewers: + primiano@chromium.org
primiano: Please review.
erikchen@chromium.org changed reviewers: + dcheng@chromium.org
dcheng: Please review the 1-line change at content/public/app/mojo/content_browser_manifest.json.
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
LGTM with some random nits https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:158: scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter( auto emitter = base::MakeShared<ProcessMemoryMetricsEmitterFake>(...) https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:52: return 0; I'm kind of surprised that there's no compiler warnings for unreachable code. Maybe put this in a #else even if there's no warnings? https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:69: process_map_(nullptr) { Nit: the default ctor does this, no need to explicitly initialize (It doesn't matter so much for std::unique_ptr, but for things like std::string(), it's nicer to use the default constructor rather than initializing it from "", etc) https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:151: auto result = process_managers_.insert(std::make_pair( Nit: consider using emplace() here to get rid of one of the nested std::make_pair calls. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:242: pmd->chrome_dump = std::move(result.second->chrome_dump); Nit: #include <utility> https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:263: // because those were comptued from the browser proces before the renderer Nit: computed https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:270: mojom::ProcessMemoryDumpPtr pmd = std::move(finalized_pmds[pid]); Nit: Maybe just alias this instead of double moving, i.e.: mojom::ProcessMemoryDumpPtr& pmd = finalized_pmds[pid]; pmd.private_footprint = CalculatePrivateFootprintKb(pmd.os_dump); global_dump->process_dumps.push_back(std:move(pmd)); https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.h:66: typedef std::pair<mojom::ProcessLocalDumpManagerPtr, Nit: prefer using to typedef https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/process_map_unittest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:11: using RunningServiceInfoPtr = service_manager::mojom::RunningServiceInfoPtr; Super duper minor nit: newline before for symmetry with } https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:54: process_map_.OnInit(std::move(v)); Nit: #include <utility>
The CQ bit was checked by erikchen@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 erikchen@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...
https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:158: scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter( On 2017/05/13 20:22:37, dcheng wrote: > auto emitter = base::MakeShared<ProcessMemoryMetricsEmitterFake>(...) Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:52: return 0; On 2017/05/13 20:22:37, dcheng wrote: > I'm kind of surprised that there's no compiler warnings for unreachable code. > Maybe put this in a #else even if there's no warnings? Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:69: process_map_(nullptr) { On 2017/05/13 20:22:37, dcheng wrote: > Nit: the default ctor does this, no need to explicitly initialize > > (It doesn't matter so much for std::unique_ptr, but for things like > std::string(), it's nicer to use the default constructor rather than > initializing it from "", etc) Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:151: auto result = process_managers_.insert(std::make_pair( On 2017/05/13 20:22:37, dcheng wrote: > Nit: consider using emplace() here to get rid of one of the nested > std::make_pair calls. Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:242: pmd->chrome_dump = std::move(result.second->chrome_dump); On 2017/05/13 20:22:37, dcheng wrote: > Nit: #include <utility> Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:263: // because those were comptued from the browser proces before the renderer On 2017/05/13 20:22:37, dcheng wrote: > Nit: computed Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:270: mojom::ProcessMemoryDumpPtr pmd = std::move(finalized_pmds[pid]); On 2017/05/13 20:22:37, dcheng wrote: > Nit: Maybe just alias this instead of double moving, i.e.: > > mojom::ProcessMemoryDumpPtr& pmd = finalized_pmds[pid]; > pmd.private_footprint = CalculatePrivateFootprintKb(pmd.os_dump); > global_dump->process_dumps.push_back(std:move(pmd)); Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/coordinator_impl.h:66: typedef std::pair<mojom::ProcessLocalDumpManagerPtr, On 2017/05/13 20:22:37, dcheng wrote: > Nit: prefer using to typedef Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... File services/resource_coordinator/memory/coordinator/process_map_unittest.cc (right): https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:11: using RunningServiceInfoPtr = service_manager::mojom::RunningServiceInfoPtr; On 2017/05/13 20:22:37, dcheng wrote: > Super duper minor nit: newline before for symmetry with } Done. https://codereview.chromium.org/2883693002/diff/40001/services/resource_coord... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:54: process_map_.OnInit(std::move(v)); On 2017/05/13 20:22:37, dcheng wrote: > Nit: #include <utility> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by erikchen@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: This issue passed the CQ dry run.
removing that NOTREACHED(); smells quite bad. Also, see comments below, I think you have a subtle UB in coordinator_impl.cc:154 https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.cc (left): https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memor... base/trace_event/memory_allocator_dump.cc:89: NOTREACHED(); Hmm why? This shouldn't really happen. If it happens some MDP is broken. if they are doing an AddString into BACKGROUND mode they will be likely doing some non-cheap traversal of their structs https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:5: #if !defined(ADDRESS_SANITIZER) any reason why we are not using the MAYBE_ pattern? never seen this in this way. Also, is this causing ASAN issues? what is the reason for this? https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:149: EXPECT_NE(0u, json_events.length()); this one won't guarantee that the dump is in the trace. Metadata events are added anyways, so even if the memory-infra event fails for some reason, you'd still pass this test. I thin you want something like: trace_analyzer::TraceEventVector events; analyzer->FindEvents(Query::EventPhaseIs(TRACE_EVENT_PHASE_MEMORY_DUMP), &events); ASSERT_EQ(1u, events.size()); ASSERT_TRUE(trace_analyzer::CountMatches( events, Query::EventNameIs(MemoryDumpTypeToString( MemoryDumpType::EXPLICITLY_TRIGGERED)))); https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:81: if (initialize_process_map) this seems only for testing. just pass nullptr as connector (ProcessMap need to support that case anyways to deal with its own unittest). https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: process_manager.get(), This relies on the evaluation order to be left-to-right, which IIRC is not specified [1]. The problem here is that if the std::move(process_manager) comes first, here you pass the .get() of an emptied object (which might be undefined behavior, depending on how the Ptr is iimplemented, std::move semantic don't guarantee that the rhs is left in a consistent state) [1] I will be honest, I don't understand what the standard says: """ From http://en.cppreference.com/w/cpp/language/eval_order: Sequenced before is an asymmetric, transitive, pair-wise relation between evaluations executed by a single thread (1.10), which induces a partial order among those evaluations. Given any two evaluations A and B, if A is sequenced before B, then the execution of A shall precede the execution of B. If A is not sequenced before B and B is not sequenced before A, then A and B are unsequenced. [ Note: The execution of unsequenced evaluations can overlap. —end note ] Evaluations A and B are indeterminately sequenced when either A is sequenced before B or B is sequenced before A, but it is unspecified which. [ Note: Indeterminately sequenced evaluations cannot overlap, but either could be executed first. —end note ] """ Also this SO seems to suggest it's not defined: http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... EDIT: dcheng@ on slack confirmed evaluation order is not well defined. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:292: void CoordinatorImpl::InitProcessMap(service_manager::Connector* connector) { as the name suggest, why this happens here and not in the ctor of ProcessMap? https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.h:34: bool initialize_process_map); don't see a strong need for this |initialize_process_map| (see the other comment in the cc file) https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.h:67: std::pair<mojom::ProcessLocalDumpManagerPtr, service_manager::Identity>; this will: 1) make the code in the .cc file full of .first.second which are unreadable 2) make it so that both the map will have to deep move all the identity objects over and over, on each rebalance operation. Identity has bunch of strings. I'd: - Create a struct which has fields, something like: struct ClientEndpoint { ClientEndpoint(service_manager::Identity, mojom::ProcessLocalDumpManagerPtr); ~ClientEndpoint(); const service_manager::Identity identity; const mojom::ProcessLocalDumpManagerPtr remote; }; use a map<PLMD*, std::unique_ptr<ClientEndpoint>> below https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.cc:21: DCHECK(!binding_.is_bound()); pass the connector and early-out if nullptr. This will require the need of the extra bool (initialize_map) in the service https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.cc:38: instances_.emplace(identity, instance->pid); i checked and, at least on Mac OS, pid at this point is always zero. I think you shouldn't do anything here, in order to avoid inserting temporary zeros, and then emplace down in OnServiceStarted. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:8: #include <unordered_map> s/unordered_map/map/ (see other comment below) https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:21: struct IdentityHasher { no need of this. Use a std::map (see brett's post about unordered-map on chromium-dev) Identity has already operator< defined, so you can just use it in a map https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:37: service_manager::mojom::ServiceManagerListenerRequest request); if you just pass the Connector in the ctor there is no need of this function https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map_unittest.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:18: class ProcessMapTest : public testing::Test { there doesn't seem to be a need for this, just s/TEST_F/TEST/ below
dskiba@chromium.org changed reviewers: + dskiba@chromium.org
https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: process_manager.get(), On 2017/05/15 03:48:09, Primiano Tucci wrote: > This relies on the evaluation order to be left-to-right, which IIRC is not > specified [1]. > The problem here is that if the std::move(process_manager) comes first, here you > pass the .get() of an emptied object (which might be undefined behavior, > depending on how the Ptr is iimplemented, std::move semantic don't guarantee > that the rhs is left in a consistent state) > > [1] I will be honest, I don't understand what the standard says: > """ > From http://en.cppreference.com/w/cpp/language/eval_order: > > Sequenced before is an asymmetric, transitive, pair-wise relation between > evaluations executed by a single thread (1.10), which induces a partial order > among those evaluations. Given any two evaluations A and B, if A is sequenced > before B, then the execution of A shall precede the execution of B. If A is not > sequenced before B and B is not sequenced before A, then A and B are > unsequenced. [ Note: The execution of unsequenced evaluations can overlap. —end > note ] Evaluations A and B are indeterminately sequenced when either A is > sequenced before B or B is sequenced before A, but it is unspecified which. [ > Note: Indeterminately sequenced evaluations cannot overlap, but either could be > executed first. —end note ] > """ > > Also this SO seems to suggest it's not defined: > http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... > > EDIT: dcheng@ on slack confirmed evaluation order is not well defined. Well, the code had the same issue before, which I think hints that map<ProcessLocalDumpManager*, ProcessLocalDumpManagerPtr> is not very friendly structure to work with. Do we really need to use map there?
On 2017/05/15 06:30:55, DmitrySkiba wrote: > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > File services/resource_coordinator/memory/coordinator/coordinator_impl.cc > (right): > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: > process_manager.get(), > On 2017/05/15 03:48:09, Primiano Tucci wrote: > > This relies on the evaluation order to be left-to-right, which IIRC is not > > specified [1]. > > The problem here is that if the std::move(process_manager) comes first, here > you > > pass the .get() of an emptied object (which might be undefined behavior, > > depending on how the Ptr is iimplemented, std::move semantic don't guarantee > > that the rhs is left in a consistent state) > > > > [1] I will be honest, I don't understand what the standard says: > > """ > > From http://en.cppreference.com/w/cpp/language/eval_order: > > > > Sequenced before is an asymmetric, transitive, pair-wise relation between > > evaluations executed by a single thread (1.10), which induces a partial order > > among those evaluations. Given any two evaluations A and B, if A is sequenced > > before B, then the execution of A shall precede the execution of B. If A is > not > > sequenced before B and B is not sequenced before A, then A and B are > > unsequenced. [ Note: The execution of unsequenced evaluations can overlap. > —end > > note ] Evaluations A and B are indeterminately sequenced when either A is > > sequenced before B or B is sequenced before A, but it is unspecified which. [ > > Note: Indeterminately sequenced evaluations cannot overlap, but either could > be > > executed first. —end note ] > > """ > > > > Also this SO seems to suggest it's not defined: > > > http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... > > > > EDIT: dcheng@ on slack confirmed evaluation order is not well defined. > > Well, the code had the same issue before, which I think hints that > map<ProcessLocalDumpManager*, ProcessLocalDumpManagerPtr> is not very friendly > structure to work with. Do we really need to use map there? Not sure what the problem is with map. Any other suggestions?
taking back some comments, will cleanup later anyways https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.h:67: std::pair<mojom::ProcessLocalDumpManagerPtr, service_manager::Identity>; On 2017/05/15 03:48:09, Primiano Tucci wrote: > this will: > > 1) make the code in the .cc file full of .first.second which are unreadable > 2) make it so that both the map will have to deep move all the identity objects > over and over, on each rebalance operation. Identity has bunch of strings. > I'd: > > - Create a struct which has fields, something like: > > struct ClientEndpoint { > ClientEndpoint(service_manager::Identity, > mojom::ProcessLocalDumpManagerPtr); > ~ClientEndpoint(); > > const service_manager::Identity identity; > const mojom::ProcessLocalDumpManagerPtr remote; > }; > > use a map<PLMD*, std::unique_ptr<ClientEndpoint>> below discussed offline, I have a patch to clean this later, so ignore this comment. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map_unittest.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map_unittest.cc:18: class ProcessMapTest : public testing::Test { On 2017/05/15 03:48:10, Primiano Tucci wrote: > there doesn't seem to be a need for this, just s/TEST_F/TEST/ below ditto on ignore, will clean up this soon anyways
On 2017/05/15 07:12:36, Primiano Tucci wrote: > On 2017/05/15 06:30:55, DmitrySkiba wrote: > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > File services/resource_coordinator/memory/coordinator/coordinator_impl.cc > > (right): > > > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: > > process_manager.get(), > > On 2017/05/15 03:48:09, Primiano Tucci wrote: > > > This relies on the evaluation order to be left-to-right, which IIRC is not > > > specified [1]. > > > The problem here is that if the std::move(process_manager) comes first, here > > you > > > pass the .get() of an emptied object (which might be undefined behavior, > > > depending on how the Ptr is iimplemented, std::move semantic don't guarantee > > > that the rhs is left in a consistent state) > > > > > > [1] I will be honest, I don't understand what the standard says: > > > """ > > > From http://en.cppreference.com/w/cpp/language/eval_order: > > > > > > Sequenced before is an asymmetric, transitive, pair-wise relation between > > > evaluations executed by a single thread (1.10), which induces a partial > order > > > among those evaluations. Given any two evaluations A and B, if A is > sequenced > > > before B, then the execution of A shall precede the execution of B. If A is > > not > > > sequenced before B and B is not sequenced before A, then A and B are > > > unsequenced. [ Note: The execution of unsequenced evaluations can overlap. > > —end > > > note ] Evaluations A and B are indeterminately sequenced when either A is > > > sequenced before B or B is sequenced before A, but it is unspecified which. > [ > > > Note: Indeterminately sequenced evaluations cannot overlap, but either could > > be > > > executed first. —end note ] > > > """ > > > > > > Also this SO seems to suggest it's not defined: > > > > > > http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... > > > > > > EDIT: dcheng@ on slack confirmed evaluation order is not well defined. > > > > Well, the code had the same issue before, which I think hints that > > map<ProcessLocalDumpManager*, ProcessLocalDumpManagerPtr> is not very friendly > > structure to work with. Do we really need to use map there? > > Not sure what the problem is with map. Any other suggestions? The problem with the map is that two versions of the code had the same bug, and apparently first time it went through unnoticed. I suggest using a vector, and do a simple linear search in UnregisterProcessLocalDumpManager (the only place where map-specific method is used).
On 2017/05/15 16:35:40, DmitrySkiba wrote: > On 2017/05/15 07:12:36, Primiano Tucci wrote: > > On 2017/05/15 06:30:55, DmitrySkiba wrote: > > > > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > > File services/resource_coordinator/memory/coordinator/coordinator_impl.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > > services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: > > > process_manager.get(), > > > On 2017/05/15 03:48:09, Primiano Tucci wrote: > > > > This relies on the evaluation order to be left-to-right, which IIRC is not > > > > specified [1]. > > > > The problem here is that if the std::move(process_manager) comes first, > here > > > you > > > > pass the .get() of an emptied object (which might be undefined behavior, > > > > depending on how the Ptr is iimplemented, std::move semantic don't > guarantee > > > > that the rhs is left in a consistent state) > > > > > > > > [1] I will be honest, I don't understand what the standard says: > > > > """ > > > > From http://en.cppreference.com/w/cpp/language/eval_order: > > > > > > > > Sequenced before is an asymmetric, transitive, pair-wise relation between > > > > evaluations executed by a single thread (1.10), which induces a partial > > order > > > > among those evaluations. Given any two evaluations A and B, if A is > > sequenced > > > > before B, then the execution of A shall precede the execution of B. If A > is > > > not > > > > sequenced before B and B is not sequenced before A, then A and B are > > > > unsequenced. [ Note: The execution of unsequenced evaluations can overlap. > > > —end > > > > note ] Evaluations A and B are indeterminately sequenced when either A is > > > > sequenced before B or B is sequenced before A, but it is unspecified > which. > > [ > > > > Note: Indeterminately sequenced evaluations cannot overlap, but either > could > > > be > > > > executed first. —end note ] > > > > """ > > > > > > > > Also this SO seems to suggest it's not defined: > > > > > > > > > > http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... > > > > > > > > EDIT: dcheng@ on slack confirmed evaluation order is not well defined. > > > > > > Well, the code had the same issue before, which I think hints that > > > map<ProcessLocalDumpManager*, ProcessLocalDumpManagerPtr> is not very > friendly > > > structure to work with. Do we really need to use map there? > > > > Not sure what the problem is with map. Any other suggestions? > > The problem with the map is that two versions of the code had the same bug, and > apparently first time it went through unnoticed. > > I suggest using a vector, and do a simple linear search in > UnregisterProcessLocalDumpManager (the only place where map-specific method is > used). I don't understand how map or vector is related here. the problem was because the two prior versions were std::move(unique_ptr) and unique_ptr.get() in the same call . It's not specific to map or anything. If you have a vector of tuple<pointer, unique_ptr, stuff> and do vector.emplace_back(unique_ptr.get(), std::move(unique_ptr), ...) you have the same bug. Not sure why we should add extra code to do something that a map does for you (is not about perf, just keeping the code simple)
On 2017/05/15 16:39:41, Primiano Tucci wrote: > On 2017/05/15 16:35:40, DmitrySkiba wrote: > > On 2017/05/15 07:12:36, Primiano Tucci wrote: > > > On 2017/05/15 06:30:55, DmitrySkiba wrote: > > > > > > > > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > > > File services/resource_coordinator/memory/coordinator/coordinator_impl.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... > > > > services/resource_coordinator/memory/coordinator/coordinator_impl.cc:154: > > > > process_manager.get(), > > > > On 2017/05/15 03:48:09, Primiano Tucci wrote: > > > > > This relies on the evaluation order to be left-to-right, which IIRC is > not > > > > > specified [1]. > > > > > The problem here is that if the std::move(process_manager) comes first, > > here > > > > you > > > > > pass the .get() of an emptied object (which might be undefined behavior, > > > > > depending on how the Ptr is iimplemented, std::move semantic don't > > guarantee > > > > > that the rhs is left in a consistent state) > > > > > > > > > > [1] I will be honest, I don't understand what the standard says: > > > > > """ > > > > > From http://en.cppreference.com/w/cpp/language/eval_order: > > > > > > > > > > Sequenced before is an asymmetric, transitive, pair-wise relation > between > > > > > evaluations executed by a single thread (1.10), which induces a partial > > > order > > > > > among those evaluations. Given any two evaluations A and B, if A is > > > sequenced > > > > > before B, then the execution of A shall precede the execution of B. If A > > is > > > > not > > > > > sequenced before B and B is not sequenced before A, then A and B are > > > > > unsequenced. [ Note: The execution of unsequenced evaluations can > overlap. > > > > —end > > > > > note ] Evaluations A and B are indeterminately sequenced when either A > is > > > > > sequenced before B or B is sequenced before A, but it is unspecified > > which. > > > [ > > > > > Note: Indeterminately sequenced evaluations cannot overlap, but either > > could > > > > be > > > > > executed first. —end note ] > > > > > """ > > > > > > > > > > Also this SO seems to suggest it's not defined: > > > > > > > > > > > > > > > http://stackoverflow.com/questions/30691551/how-to-determine-c-function-call-... > > > > > > > > > > EDIT: dcheng@ on slack confirmed evaluation order is not well defined. > > > > > > > > Well, the code had the same issue before, which I think hints that > > > > map<ProcessLocalDumpManager*, ProcessLocalDumpManagerPtr> is not very > > friendly > > > > structure to work with. Do we really need to use map there? > > > > > > Not sure what the problem is with map. Any other suggestions? > > > > The problem with the map is that two versions of the code had the same bug, > and > > apparently first time it went through unnoticed. > > > > I suggest using a vector, and do a simple linear search in > > UnregisterProcessLocalDumpManager (the only place where map-specific method is > > used). > > I don't understand how map or vector is related here. > the problem was because the two prior versions were std::move(unique_ptr) and > unique_ptr.get() in the same call . It's not specific to map or anything. > If you have a vector of tuple<pointer, unique_ptr, stuff> and do > vector.emplace_back(unique_ptr.get(), std::move(unique_ptr), ...) you have the > same bug. > Not sure why we should add extra code to do something that a map does for you > (is not about perf, just keeping the code simple) The map in question was std::unordered_map<mojom::ProcessLocalDumpManager*, mojom::ProcessLocalDumpManagerPtr> Inserting unique_ptr (ProcessLocalDumpManagerPtr) will always be messy, because you have to (1) extract pointer from unique_ptr and (2) move the unique_ptr. This patch changes it to std::unordered_map<ProcessLocalDumpManager*, ProcessLocalDumpManagerEntry>. The issue with insertion is still there, and it's even more complicated now (you have two nested pairs). I'm suggesting replacing the map with simple std::vector<ProcessLocalDumpManagerEntry>, avoiding the issue with extracting the value from / moving the unique_ptr - you'll just do a single move, nice and simple. Especially if performance of UnregisterProcessLocalDumpManager() is not an issue.
The CQ bit was checked by erikchen@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 erikchen@chromium.org to run a CQ dry run
primiano: PTAL https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memor... File base/trace_event/memory_allocator_dump.cc (left): https://codereview.chromium.org/2883693002/diff/120001/base/trace_event/memor... base/trace_event/memory_allocator_dump.cc:89: NOTREACHED(); On 2017/05/15 03:48:09, Primiano Tucci wrote: > Hmm why? This shouldn't really happen. If it happens some MDP is broken. > if they are doing an AddString into BACKGROUND mode they will be likely doing > some non-cheap traversal of their structs Done. https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc (right): https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:5: #if !defined(ADDRESS_SANITIZER) On 2017/05/15 03:48:09, Primiano Tucci wrote: > any reason why we are not using the MAYBE_ pattern? never seen this in this way. > Also, is this causing ASAN issues? what is the reason for this? I wrapped the only relevant call in a "!defined(ADDRESS_SANTIZER)." block. https://codereview.chromium.org/2883693002/diff/120001/chrome/browser/metrics... chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc:149: EXPECT_NE(0u, json_events.length()); On 2017/05/15 03:48:09, Primiano Tucci wrote: > this one won't guarantee that the dump is in the trace. Metadata events are > added anyways, so even if the memory-infra event fails for some reason, you'd > still pass this test. > > I thin you want something like: > > trace_analyzer::TraceEventVector events; > analyzer->FindEvents(Query::EventPhaseIs(TRACE_EVENT_PHASE_MEMORY_DUMP), > &events); > > ASSERT_EQ(1u, events.size()); > ASSERT_TRUE(trace_analyzer::CountMatches( > events, Query::EventNameIs(MemoryDumpTypeToString( > MemoryDumpType::EXPLICITLY_TRIGGERED)))); > Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:81: if (initialize_process_map) On 2017/05/15 03:48:09, Primiano Tucci wrote: > this seems only for testing. just pass nullptr as connector (ProcessMap need to > support that case anyways to deal with its own unittest). Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:292: void CoordinatorImpl::InitProcessMap(service_manager::Connector* connector) { On 2017/05/15 03:48:09, Primiano Tucci wrote: > as the name suggest, why this happens here and not in the ctor of ProcessMap? moved https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/coordinator_impl.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/coordinator_impl.h:34: bool initialize_process_map); On 2017/05/15 03:48:10, Primiano Tucci wrote: > don't see a strong need for this |initialize_process_map| (see the other comment > in the cc file) Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map.cc (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.cc:21: DCHECK(!binding_.is_bound()); On 2017/05/15 03:48:10, Primiano Tucci wrote: > pass the connector and early-out if nullptr. This will require the need of the > extra bool (initialize_map) in the service Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.cc:38: instances_.emplace(identity, instance->pid); On 2017/05/15 03:48:10, Primiano Tucci wrote: > i checked and, at least on Mac OS, pid at this point is always zero. I think you > shouldn't do anything here, in order to avoid inserting temporary zeros, and > then emplace down in OnServiceStarted. I added an early return to check for kNullProcessId. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... File services/resource_coordinator/memory/coordinator/process_map.h (right): https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:8: #include <unordered_map> On 2017/05/15 03:48:10, Primiano Tucci wrote: > s/unordered_map/map/ (see other comment below) Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:21: struct IdentityHasher { On 2017/05/15 03:48:10, Primiano Tucci wrote: > no need of this. Use a std::map (see brett's post about unordered-map on > chromium-dev) > Identity has already operator< defined, so you can just use it in a map Done. https://codereview.chromium.org/2883693002/diff/120001/services/resource_coor... services/resource_coordinator/memory/coordinator/process_map.h:37: service_manager::mojom::ServiceManagerListenerRequest request); On 2017/05/15 03:48:10, Primiano Tucci wrote: > if you just pass the Connector in the ctor there is no need of this function Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org, avi@chromium.org ==========
erikchen@chromium.org changed reviewers: + avi@chromium.org, rkaplow@chromium.org
TBR avi for moving code around in content/browser/browser_main_loop.cc. TBR rkaplow for tiny changes to chrome/browser/metrics/process_memory_metrics_emitter.cc, more comprehensive tests, and tiny change to tools/metrics/histograms/histograms.xml.
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org, avi@chromium.org ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org ==========
On 2017/05/15 17:48:18, erikchen wrote: > TBR avi for moving code around in content/browser/browser_main_loop.cc. > > TBR rkaplow for tiny changes to > chrome/browser/metrics/process_memory_metrics_emitter.cc, more comprehensive > tests, and tiny change to tools/metrics/histograms/histograms.xml. un-TBRing avi on the premise that he's very responsive.
lgtm content stampity stamp
LGTM
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2883693002/#ps160001 (title: "fix macro.")
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 erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, avi@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2883693002/#ps180001 (title: "Disable test on ASAN.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org
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": 180001, "attempt_start_ts": 1494891138518500,
"parent_rev": "d516a06227530ca47ccf5f872b334326e0137221", "commit_rev":
"18ea873dd28c6b2f49b529e57c55898f5df1b4d2"}
Message was sent while issue was closed.
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#471964} Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 471964 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/05/16 02:47:36, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 471964 as the > culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... For any future visitors - looks like this was reverted in r471985. Details - http://crbug.com/722673 .
Message was sent while issue was closed.
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#471964} Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... ========== to ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#471964} Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... ==========
lgtm
Description was changed from ========== [Memory-UMA] Implement basic working prototype. This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. This CL includes the following changes: * Fix histogram emissions to not be off by a factor of 2 ** 20. * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the scope of end-to-end tests to sanity check UMA emissions. * Change the location where the object that backs the memory instrumentation interface is created, as it now requires Mojo to be initialized. * The implementation of the memory instrumentation interface now requires ServiceManagerListener, so add that requirement to content_browser_manifest.json. * Create a ProcessMap class that maps service identifiers to pids. * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. BUG=703184 TBR=rkaplow@chromium.org Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#471964} Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... ========== to ========== [reland 1] [Memory-UMA] Implement basic working prototype. The first attempt to land this CL had test failures on ChromeOS for the newly added tests. The tests have been updated to also work on ChromeOS. > This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. > > This CL includes the following changes: > * Fix histogram emissions to not be off by a factor of 2 ** 20. > * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the > scope of end-to-end tests to sanity check UMA emissions. > * Change the location where the object that backs the memory instrumentation > interface is created, as it now requires Mojo to be initialized. > * The implementation of the memory instrumentation interface now requires > ServiceManagerListener, so add that requirement to > content_browser_manifest.json. > * Create a ProcessMap class that maps service identifiers to pids. > * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. > > BUG=703184 > TBR=rkaplow@chromium.org > > Review-Url: https://codereview.chromium.org/2883693002 > Cr-Commit-Position: refs/heads/master@{#471964} > Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... BUG=703184 ==========
The CQ bit was checked by erikchen@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 erikchen@chromium.org to run a CQ dry run
The CQ bit was unchecked by erikchen@chromium.org
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, avi@chromium.org, rkaplow@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2883693002/#ps240001 (title: "Fix test.")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org
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": 240001, "attempt_start_ts": 1495070566301260,
"parent_rev": "169c4a19654509f37de918d8aee9df4ad343cc2a", "commit_rev":
"fefc7cd8a1677036dca0c7ed55fc1c72f683c16d"}
Message was sent while issue was closed.
Description was changed from ========== [reland 1] [Memory-UMA] Implement basic working prototype. The first attempt to land this CL had test failures on ChromeOS for the newly added tests. The tests have been updated to also work on ChromeOS. > This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. > > This CL includes the following changes: > * Fix histogram emissions to not be off by a factor of 2 ** 20. > * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the > scope of end-to-end tests to sanity check UMA emissions. > * Change the location where the object that backs the memory instrumentation > interface is created, as it now requires Mojo to be initialized. > * The implementation of the memory instrumentation interface now requires > ServiceManagerListener, so add that requirement to > content_browser_manifest.json. > * Create a ProcessMap class that maps service identifiers to pids. > * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. > > BUG=703184 > TBR=rkaplow@chromium.org > > Review-Url: https://codereview.chromium.org/2883693002 > Cr-Commit-Position: refs/heads/master@{#471964} > Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... BUG=703184 ========== to ========== [reland 1] [Memory-UMA] Implement basic working prototype. The first attempt to land this CL had test failures on ChromeOS for the newly added tests. The tests have been updated to also work on ChromeOS. > This CL is based on fmeawad's CL at https://codereview.chromium.org/2875673003/. > > This CL includes the following changes: > * Fix histogram emissions to not be off by a factor of 2 ** 20. > * Add several end-to-end tests for ProcessMemoryMetricsEmitter. Expand the > scope of end-to-end tests to sanity check UMA emissions. > * Change the location where the object that backs the memory instrumentation > interface is created, as it now requires Mojo to be initialized. > * The implementation of the memory instrumentation interface now requires > ServiceManagerListener, so add that requirement to > content_browser_manifest.json. > * Create a ProcessMap class that maps service identifiers to pids. > * Hook up the plumbing in CoordinatorImpl to correctly assemble dumps. > > BUG=703184 > TBR=rkaplow@chromium.org > > Review-Url: https://codereview.chromium.org/2883693002 > Cr-Commit-Position: refs/heads/master@{#471964} > Committed: https://chromium.googlesource.com/chromium/src/+/18ea873dd28c6b2f49b529e57c55... BUG=703184 Review-Url: https://codereview.chromium.org/2883693002 Cr-Commit-Position: refs/heads/master@{#472659} Committed: https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc...
Message was sent while issue was closed.
On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > Committed patchset #13 (id:240001) as > https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc... The new tests fail on MSan bots: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%... https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom... For some reason Rietveld doesn't want to revert ("file too large"?!), so I'll revert manually.
Message was sent while issue was closed.
On 2017/05/18 13:54:25, Marc Treib wrote: > On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > > Committed patchset #13 (id:240001) as > > > https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc... > > The new tests fail on MSan bots: > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%... > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom... > > For some reason Rietveld doesn't want to revert ("file too large"?!), so I'll > revert manually. ...or I won't, since the revert already doesn't apply cleanly anymore. I guess I'll just disable the tests on MSan...
Message was sent while issue was closed.
On 2017/05/18 13:59:18, Marc Treib wrote: > On 2017/05/18 13:54:25, Marc Treib wrote: > > On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > > > Committed patchset #13 (id:240001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc... > > > > The new tests fail on MSan bots: > > > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%... > > > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom... > > > > For some reason Rietveld doesn't want to revert ("file too large"?!), so I'll > > revert manually. > > ...or I won't, since the revert already doesn't apply cleanly anymore. I guess > I'll just disable the tests on MSan... CL to disable the tests on MSan: https://codereview.chromium.org/2891063002/
Message was sent while issue was closed.
On 2017/05/18 14:01:58, Marc Treib wrote: > On 2017/05/18 13:59:18, Marc Treib wrote: > > On 2017/05/18 13:54:25, Marc Treib wrote: > > > On 2017/05/18 04:22:01, commit-bot: I haz the power wrote: > > > > Committed patchset #13 (id:240001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/fefc7cd8a1677036dca0c7ed55fc... > > > > > > The new tests fail on MSan bots: > > > > > > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%... > > > > > > https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chrom... > > > > > > For some reason Rietveld doesn't want to revert ("file too large"?!), so > I'll > > > revert manually. > > > > ...or I won't, since the revert already doesn't apply cleanly anymore. I guess > > I'll just disable the tests on MSan... > > CL to disable the tests on MSan: https://codereview.chromium.org/2891063002/ Thanks - I took a look at the failures and it looks like disabling the test for MSAN is the right behavior. The failures are because we're failing to fetch memory related metrics, which is expected in MSAN. |
