Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1289793007:
Allow unregistering MemoryDumpProviders during dump (Closed)
|
Created:
5 years, 4 months ago by Ruud van Asseldonk Modified:
5 years, 4 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow unregistering MemoryDumpProviders during dump
Previously, if a memory dump provider would unregister itself while a
dump was in progress, this would abort the dump. While the scenario is
unlikely, it was triggered in some tests (https://crbug.com/522009).
Instead of unregistering immediately, we now set a flag and skip the
provider during dump if it has been unregistered. At the end of every
dump, we remove all unregistered providers. When the registration
includes a task runner, the refptr can keep the task runner alive until
the next dump even when the provider has unregistered itself.
BUG=518823, 522009, 522165
Committed: https://crrev.com/535f0876943ebd615b8a736cc96ecbae155a7919
Cr-Commit-Position: refs/heads/master@{#345057}
Patch Set 1 : Retain and flag dump provider on unregistration #Patch Set 2 : Ensure retaining does not affect behavior #Patch Set 3 : Do not (un)register with tracing enabled #
Total comments: 22
Patch Set 4 : Address review issues #
Total comments: 14
Patch Set 5 : Address review issues #Patch Set 6 : Replace auto with explicit types #
Dependent Patchsets: Messages
Total messages: 16 (6 generated)
ruuda@google.com changed reviewers: + dsinclair@chromium.org, primiano@chromium.org
This should fix issue 522009. I am not sure whether relanding the tests should go in its own CL, let me know if this needs to be split. The last two patchsets have been reviewed already at https://crrev.com/1290103004.
On 2015/08/20 13:41:10, Ruud van Asseldonk wrote: > This should fix issue 522009. I am not sure whether relanding the tests should > go in its own CL, let me know if this needs to be split. The last two patchsets > have been reviewed already at https://crrev.com/1290103004. Let's split them. That way, if needed, the tests can be reverted without effecting the code.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Many thanks for putting this CL together and the thorough testing! Make sense overall, but I've some comments here and there. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:156: // If there was a previous entry, replace it with the new one. Is this really necessary? To me the fact that something can doubly-register is a bug. we should just hard fail in that case. What I'd do here is auto iter_new = dump_providers_.insert(mdp_info); const bool did_insert = iter_new.second; DCHECK(did_insert); And NOT handle gracefully the erase + reinsert below. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:300: mdp_info->disabled = true; I wonder at this point if we should also gracefully handle this other case (in another CL) https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:328: // Remove providers that unregistered since the last dump. Hmm I don't like too much the idea of iterating over the full set of dump providers on every dump. I think we can do something more efficient here. My proposal: 1. Turn mdp_info into an iterator (remove &* on line 275), not a ptr (now it's safe to do that as you are guaranteed you never remove anything outside of this function). 2. On line 325 do: ++pmd_async_state->next_dump_provider; if (mdp_info->unregistered) dump_providers_.erase(mdp_info) // Nothing below this point should use mdp_info. This should always be safe and from a performance viewpoint will be: 1) Just a if check for most cases (if you don't unregister anything) 2) A straight removal from the set, only if you need to actually unregister https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:470: bool MemoryDumpManager::MemoryDumpProviderInfo::operator==( Why is this needed? AFAIK std::set relies only on the < operator. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:133: mutable bool unregistered; I think you don't need this to be mutable. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:287: // Even though internally, after unregistration a dump provider is retained in This comment seems to describe the internal implementation of MDM. Instead it should reflect what this test is doing and what is protecting from. How about something like: "Checks that Unregistration of a MemoryDumpProvider has always effect regardless of the state of the trace and ongoing dumps" And I'd probably name the test "RegistrationConsistency" as you aren't really checking any ordering here, right? (unless I'm misreading this) https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:492: int on_memory_dump_calls = 0; nit: +count (i.e. on_memory_dump_call_count) https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:499: auto on_dump = [&, other_idx](const MemoryDumpArgs& args, nit: on_memory_dump Also, make sure you run git cl format on this, the formatting of this line looks a bit strange (but might be correct) https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:512: .Times(AtLeast(0)) I'd probably make this AtMost(1) https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:533: // Thread destructors will stop the threads. I'd probably remove this comment. Should be clear from the "Scoped" name
https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:156: // If there was a previous entry, replace it with the new one. On 2015/08/21 09:00:31, Primiano Tucci wrote: > Is this really necessary? To me the fact that something can doubly-register is a > bug. > we should just hard fail in that case. What I'd do here is > > auto iter_new = dump_providers_.insert(mdp_info); > const bool did_insert = iter_new.second; > DCHECK(did_insert); > > And NOT handle gracefully the erase + reinsert below. If the provider registers itself twice, that is a bug, but if a provider unregisters and then registers again, the entry might still be there, with `unregistered` set to true. If it is not expected for providers to ever register again after unregistering then this is redundant, or we could add an assertion. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:300: mdp_info->disabled = true; On 2015/08/21 09:00:31, Primiano Tucci wrote: > I wonder at this point if we should also gracefully handle this other case (in > another CL) Does it happen in practice? https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:328: // Remove providers that unregistered since the last dump. On 2015/08/21 09:00:30, Primiano Tucci wrote: > Hmm I don't like too much the idea of iterating over the full set of dump > providers on every dump. I think we can do something more efficient here. > > My proposal: > > 1. Turn mdp_info into an iterator (remove &* on line 275), not a ptr (now it's > safe to do that as you are guaranteed you never remove anything outside of this > function). > > 2. On line 325 do: > ++pmd_async_state->next_dump_provider; > if (mdp_info->unregistered) > dump_providers_.erase(mdp_info) > // Nothing below this point should use mdp_info. > > This should always be safe and from a performance viewpoint will be: > 1) Just a if check for most cases (if you don't unregister anything) > 2) A straight removal from the set, only if you need to actually unregister Done. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:470: bool MemoryDumpManager::MemoryDumpProviderInfo::operator==( On 2015/08/21 09:00:30, Primiano Tucci wrote: > Why is this needed? AFAIK std::set relies only on the < operator. It isn’t needed. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:133: mutable bool unregistered; On 2015/08/21 09:00:31, Primiano Tucci wrote: > I think you don't need this to be mutable. I does need to be mutable, for the same reason that `disabled` and `consecutive_failures` are; you cannot modify the entry through an iterator, the iterator is const because changing an element might affect the ordering (in this case it does not). The normal approach is to erase the entry, modify it and then insert it again, but the point of the variable is to avoid modifying the set. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:287: // Even though internally, after unregistration a dump provider is retained in On 2015/08/21 09:00:31, Primiano Tucci wrote: > This comment seems to describe the internal implementation of MDM. Instead it > should reflect what this test is doing and what is protecting from. > How about something like: > "Checks that Unregistration of a MemoryDumpProvider has always effect regardless > of the state of the trace and ongoing dumps" > > And I'd probably name the test "RegistrationConsistency" as you aren't really > checking any ordering here, right? (unless I'm misreading this) Done. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:492: int on_memory_dump_calls = 0; On 2015/08/21 09:00:31, Primiano Tucci wrote: > nit: +count (i.e. on_memory_dump_call_count) Done. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:499: auto on_dump = [&, other_idx](const MemoryDumpArgs& args, On 2015/08/21 09:00:31, Primiano Tucci wrote: > nit: on_memory_dump > > Also, make sure you run git cl format on this, the formatting of this line looks > a bit strange (but might be correct) This is how git cl format formats it. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:512: .Times(AtLeast(0)) On 2015/08/21 09:00:31, Primiano Tucci wrote: > I'd probably make this AtMost(1) Done. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:533: // Thread destructors will stop the threads. On 2015/08/21 09:00:31, Primiano Tucci wrote: > I'd probably remove this comment. Should be clear from the "Scoped" name Done.
The production code looks great (% some comment on comments here and there). We just some fixes to the test to fix the lambda default capture. https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:300: mdp_info->disabled = true; On 2015/08/21 10:51:40, Ruud van Asseldonk wrote: > On 2015/08/21 09:00:31, Primiano Tucci wrote: > > I wonder at this point if we should also gracefully handle this other case (in > > another CL) > > Does it happen in practice? I don't know, that was I was wondering. It's just that one speculation of mine "In the unlikely event" turned out to be not so unlikely. So I wonder whether this is like that :) Anyways, this doesn't belong to the current CL . https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1289793007/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:133: mutable bool unregistered; On 2015/08/21 10:51:40, Ruud van Asseldonk wrote: > On 2015/08/21 09:00:31, Primiano Tucci wrote: > > I think you don't need this to be mutable. > > I does need to be mutable, for the same reason that `disabled` and > `consecutive_failures` are; you cannot modify the entry through an iterator, the > iterator is const because changing an element might affect the ordering (in this > case it does not). The normal approach is to erase the entry, modify it and then > insert it again, but the point of the variable is to avoid modifying the set. Ahh right. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:156: // If there was a previous entry, replace it with the new one. Please add a comment here: // This is to deal with the corner case of a dump provider unregistering and re-registering immediately after. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:328: if (mdp_info->unregistered) { tip: you can omit the braces for single line ifs if you want. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:129: // When unregistering, we do not remove the provider immediately because a Please remove the "we".As a general rule it usually doesn't add too much informative content and is ambiguos (who is we? users? developers? tracing owners?) Also I'r reword this comment a bit, the main problem here is not that the dump depends on the set order. Is that we want to avoid changing the dump_providers_ in the middle of a dump. I'd probably say here When a dump provider unregisters, it is flagged as |unregistered| and is removed only upon the next memory dump. This is to avoid altering the |dump_providers_| collection while a dump is in progress. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:295: EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1); I wonder if you should put all these macro-steps into braces, in order to limit the scope of EXPECT_CALL and give it the semantic you intend. In other words, I'm not sure that EXPECT_CALL(Foo).Times(1) do_something() EXPECT_CALL(Foo).Times(0) do_something() is equivalent to { EXPECT_CALL(Foo).Times(1) do_something() } { EXPECT_CALL(Foo).Times(0) do_something() } https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:480: // Verify that the dump does not abort when unregistering a provider during dump s/during dump/while dumping/ https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:499: auto on_dump = [&, other_idx](const MemoryDumpArgs& args, According to our C++11 codestyle [1], "Don't use default captures ([=], [&] " [1] https://chromium-cpp.appspot.com/ https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:516: last_callback_success_ = true; shouldn't you set this to false to make sure you actually get a callback that sets it to true?
https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:156: // If there was a previous entry, replace it with the new one. On 2015/08/24 09:31:19, Primiano Tucci wrote: > Please add a comment here: > // This is to deal with the corner case of a dump provider unregistering and > re-registering immediately after. Done. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:328: if (mdp_info->unregistered) { On 2015/08/24 09:31:19, Primiano Tucci wrote: > tip: you can omit the braces for single line ifs if you want. Done. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:129: // When unregistering, we do not remove the provider immediately because a On 2015/08/24 09:31:19, Primiano Tucci wrote: > Please remove the "we".As a general rule it usually doesn't add too much > informative content and is ambiguos (who is we? users? developers? tracing > owners?) > Also I'r reword this comment a bit, the main problem here is not that the dump > depends on the set order. Is that we want to avoid changing the dump_providers_ > in the middle of a dump. > I'd probably say here > > When a dump provider unregisters, it is flagged as |unregistered| and is removed > only upon the next memory dump. This is to avoid altering the |dump_providers_| > collection while a dump is in progress. Done. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:295: EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1); On 2015/08/24 09:31:19, Primiano Tucci wrote: > I wonder if you should put all these macro-steps into braces, in order to limit > the scope of EXPECT_CALL and give it the semantic you intend. > In other words, I'm not sure that > > EXPECT_CALL(Foo).Times(1) > do_something() > > EXPECT_CALL(Foo).Times(0) > do_something() > > is equivalent to > { > EXPECT_CALL(Foo).Times(1) > do_something() > } > { > EXPECT_CALL(Foo).Times(0) > do_something() > } As I understand from the documentation, in this case a call to OnMemoryDump should trigger the most recently set expectation. Still, it won’t hurt to be more explicit so I added the scopes. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:480: // Verify that the dump does not abort when unregistering a provider during dump On 2015/08/24 09:31:19, Primiano Tucci wrote: > s/during dump/while dumping/ Done. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:499: auto on_dump = [&, other_idx](const MemoryDumpArgs& args, On 2015/08/24 09:31:19, Primiano Tucci wrote: > According to our C++11 codestyle [1], "Don't use default captures ([=], [&] " > > [1] https://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/1289793007/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:516: last_callback_success_ = true; On 2015/08/24 09:31:19, Primiano Tucci wrote: > shouldn't you set this to false to make sure you actually get a callback that > sets it to true? Definitely, good catch.
LGTM after reducing the auto magic as discussed offline.
The CQ bit was checked by ruuda@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1289793007/#ps160001 (title: "Replace auto with explicit types")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1289793007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1289793007/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/535f0876943ebd615b8a736cc96ecbae155a7919 Cr-Commit-Position: refs/heads/master@{#345057} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
