Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1430073002:
[tracing] Allow asynchronous unregistration of unbound dump providers (Closed)
|
Created:
5 years, 1 month ago by Primiano Tucci (use gerrit) Modified:
4 years, 11 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Allow asynchronous unregistration of unbound dump providers
Before this change, it was impossible to unregister unbound (i.e. no
task runner affinity) dump providers. The reason is that unbound dump
providers are invoked on a utility thread, which is transparent to the
clients. Therefore the client cannot possibly guarantee that the
unregistration will not race with the OnMemoryDump call.
This CL solves the problem introducing a
UnregisterAndDeleteSoonDumpProvider() method. The method takes
ownership of the dump provider and guarantees that the dump provider
will be deleted at some point in the near future (even immediately)
and its deletion will be guaranteed to not overlap with the
OnMemoryDump() call.
BUG=461788
Committed: https://crrev.com/4c70669ee6f7bc1fccb61b21b89aa0c83d5c40a5
Cr-Commit-Position: refs/heads/master@{#367505}
Patch Set 1 #Patch Set 2 : #
Total comments: 19
Patch Set 3 : new version #Patch Set 4 : add test #Patch Set 5 : fix doc #
Total comments: 25
Patch Set 6 : ruuda + ssid review #
Total comments: 3
Patch Set 7 : Rename method #
Total comments: 2
Patch Set 8 : avoid repeating messages #
Messages
Total messages: 26 (9 generated)
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers WORK IN PROGRESS DO NOT REVIEW YET BUG= ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers TODO: improve this commit message before landing. - Allow asynchronous unregistration of unbound dump providers - Make the manager more robust in case of threads disappearing (also simplifies the code). - Add a test for the latter. BUG= ==========
primiano@chromium.org changed reviewers: + ruuda@google.com, ssid@chromium.org
I feel that this is far more complex than it needs to be. The code is fighting |scoped_ptr|s instead of letting them do the work. How about the following: * Make |MemoryDumpProvider| atomically refcounted. In the end the goal is to keep the dump provider alive if we are still going to use it, so the manager should be one of the owners. This removes the need for |UnregisterAndDeleteDumpProviderAsync|. * Copy all currently registered dump providers into |ProcessMemoryDumpAsyncState| when starting the dump. Now providers can be safely unregistered from the memory dump provider even when a dump is in progress. If we allow unregistering from one thread while a dump is happening on a different thread we cannot guarantee that a provider is not used after unregistering it anyway, so don’t even bother to try. Unregister will remove from the collection, and |RequestGlobalDump| will visit all providers that were registered at the time |RequestGlobalDump| was called. * Make |ProcessMemoryDumpAsyncState| atomically refcounted. Make it do finalisation and post the callback from its destructor. Do the |PostTask| to hop onto the next thread in a loop, but break after one succeeds. (You can also make it uniquely owned but then you get into the current awkwardness with |PostTask| eating it on failure. Alternatively we could fail the entire dump if a |PostTask| fails, and don’t bother to try and continue.) * There is no need for |outstanding_dumps_|. * There is no need for |MemoryDumpProviderInfo::unregistered|. * There is no need for |AbortDumpLocked|. * There is no need to keep a |finalize| flag in |ContinueAsyncProcessDump|. * The only thing that needs lock protection is the collection of dump providers. * Thread affinity (with the dumper thread if not specified) ensures that |OnMemoryDump| is never called in parallel for a dump provider. What do you think? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:249: return; // Already unregistered. So the async deletion might actually be synchronous? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:274: FROM_HERE, Bind(&DeleteMemoryDumpProvider, Passed(&owned_mdp))); Having an in-flight message own the dump provider feels wrong. Perhaps it would be better to have a container of owned dump providers in the async state, so they will go out of scope together with the async state? Then you won’t need |DeleteMemoryDumpProvider| which is a bit awkward. The dump provider does not care on which thread it is deleted anyway. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:335: outstanding_dumps_.push_back(pmd_async_state.get()); By doing this you lose all of the advantages of a |scoped_ptr|, because now you need to ensure that the pointer in |outstanding_dumps_| does not become stale. In all places where you would have to call delete if this were a naked pointer, you now have to remove the pointer from this vector. The STL version is called |unique_ptr| for a reason. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:375: // which needs to take back the ownsership of the |pmd_async_state| when a /s/ownsership/ownership/ https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:379: auto pmd_async_state = make_scoped_ptr(owned_pmd_async_state); I wonder if it would be simpler to just make |pmd_async_state| refcounted. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:419: // Copy the callback + arguments just for the unlikley case in which /s/unlikley/unlikely/ https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:423: MemoryDumpCallback callback = pmd_async_state->callback; This variable is now unused. (Didn't Clang warn about that?) https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:424: scoped_refptr<SingleThreadTaskRunner> callback_task_runner = Same here. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:427: const bool did_post_task = task_runner->PostTask( From the |PostTask| documentation, this is no guarantee that you don't leak |pmd_async_state|. > Returns true if the task may be run at some > point in the future, and false if the task > definitely will not be run. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:433: (void)pmd_async_state.release(); You can do |pmd_async_state.reset()| instead. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:439: mdp_info->disabled = true; Shouldn't all dump providers that share that thread be disabled? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:491: ContinueAsyncProcessDump(std::move(pmd_async_state.release())); You don't need the move here. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:89: // - Its deletion will not happen concurrently with the OnMemoryDump() call. /s/concurrently/in parallel/? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:91: void UnregisterAndDeleteDumpProviderAsync(scoped_ptr<MemoryDumpProvider> mdp); This feels like an awkward API. It looks like what you really want is shared ownership; the memory dump manager must be able to keep the dump provider alive. Would it be possible to make |MemoryDumpProvider| atomically refcounted instead? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:253: bool delete_async); This is ambiguous; does |delete_async = false| mean "delete synchronously" or does it mean "do not delete at all"? Instead of taking a boolean argument, consider having an enum enum class DeletionMode { kDoNotDelete, kDeleteAsync }; It is more readable at the call site too. https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:267: // Contains one entry per each dump that has been started (via Nit: "per" or "for each", not "per each". https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:269: // Only the const members of the stores ProcessMemoryDumpAsyncState are safe /s/stores/stored/? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:270: // to read and only while holding the |lock_|. That is a violation of the general |const| contract. Since C++11 |const| means thread safe (bitwise const or internally synchronised). This talk by Herb Sutter explains it in detail: https://channel9.msdn.com/posts/C-and-Beyond-2012-Herb-Sutter-You-dont-know-b... Also, does this mean that the non-const members are _never_ safe to read? https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1430073002/diff/20001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:586: ScopedVector<TestIOThread> threads; Scoped vectors are being removed in favour of |std::vector<scoped_ptr<T>>|.
On 2015/12/14 16:25:49, Ruud van Asseldonk wrote: > I feel that this is far more complex than it needs to be. You learnt too well too soon :) > The code is fighting > |scoped_ptr|s instead of letting them do the work. Happy to find a more elegant solution. > How about the following: > * Make |MemoryDumpProvider| atomically refcounted. In the end the goal is to > keep the dump provider alive if we are still going to use it, so the manager > should be one of the owners. This removes the need for > |UnregisterAndDeleteDumpProviderAsync|. Nope, for two reasons: 1) making MDP refcounted means changing (and re-thinking) 20-30 clients in the codebase that implement that interface. MDP is an interface, if I make it refcounted, it means that I force all those classes to be refcounted as well. This will not fly. I just want to keep some dump providers (the one that cannot handle their own memory ownership themselves) alive for the time it takes to delete them properly. Which is the reason why I am introducing this in the first place: I need a way to delete unbound dump providers in a safe way. Any better alternative here would make me happy. > * Copy all currently registered dump providers into > |ProcessMemoryDumpAsyncState| when starting the dump. Now providers can be > safely unregistered from the memory dump provider even when a dump is in > progress. If we allow unregistering from one thread while a dump is happening > on a different thread we cannot guarantee that a provider is not used after > unregistering it anyway, so don’t even bother to try. Unregister will remove > from the collection, and |RequestGlobalDump| will visit all providers that > were registered at the time |RequestGlobalDump| was called. Which is wrong. Most of the times the bound (the one with task affinity) unregister on their destruction. There is no way we can cache them and call them once we return from the Unregister call. > > * Make |ProcessMemoryDumpAsyncState| atomically refcounted. Hmm refounted things are harder to reason about and more prone to bugs. You can end up with something mistakenly extending their lifetime more than necessary. > Make it do finalisation and post the callback from its destructor. That's a very odd pattern (doing useful stuff on destructor) and very error prone: you need to start thinking to destruction order. No way here :) > Do the |PostTask| to > hop onto the next thread in a loop, but break after one succeeds. (You can > also make it uniquely owned but then you get into the current awkwardness > with |PostTask| eating it on failure. Alternatively we could fail the entire > dump if a |PostTask| fails, and don’t bother to try and continue.) Not sure I follow this. > * There is no need for |AbortDumpLocked|. This is orthogonal, and I'm cleaning up just here because I'm realizing this now. Can be done anyways, agree. Maybe let's discuss offline the rest.
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers TODO: improve this commit message before landing. - Allow asynchronous unregistration of unbound dump providers - Make the manager more robust in case of threads disappearing (also simplifies the code). - Add a test for the latter. BUG= ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the solution introducing a UnregisterAndDeleteDumpProviderAsync() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ==========
ssid@ can you take a second look. I went for the Unregister...Async version. ruuda@ I know that you'll not like this, but I thought more and I really think that we shouldn't silently take locks and block message loops, especially if there are other ways of handling this.
oh just realized this needs a fix in the condition below before landing. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:256: subtle::NoBarrier_Load(&memory_tracing_enabled_)) { ah this logic is wrong. The else below should be: else if (take_ownership) ... or something equivalent
https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:418: LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, " Isn't logging inside lock slow? https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:419: << "possibly due to sandboxing (crbug.com/461788)." Do you think I should remove this line in my CL, or at least change the log statement. At that point this if conditions can be merged. and log can be made common, something like: "MemoryDumpProvider %s is disabled due to multiple failures or missing thread" https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:426: // The MDP might have been unregistered while we were hopping threads. I think this comment is not needed. Moreover the mdp could have been unregistered while dumping on other threads probably? Also the else condition is not needed. I think the code should be: if (consecutive_failures >= kMaxConsecutiveFailuresCount or post_task_failed) { mdpinfo->disabled = true; LOG(ERROR) << "MemoryDumpProvider %s is disabled due to multiple failures or missing thread"; } skip_dump = mdpinfo->disabled; But if you are keen on adding log only for that case, you could have 2 conditions. else is still not needed. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:431: if (!skip_dump) { I wonder why shouldn't this be should_dump or similarly positive. You are checking for ! of negative here. I get confused every time i read this code.
one more nit. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:913: // from another thread while dumping. It register one MDP and when s/register/registers/
On 2015/12/22 13:46:55, Primiano Tucci (OOO til Jan 4) wrote: > ssid@ can you take a second look. > I went for the Unregister...Async version. > > ruuda@ I know that you'll not like this, but I thought more and I really > think that we shouldn't silently take locks and block message loops, > especially if there are other ways of handling this. I don’t like the idea of the MDM having to care about ownership and special-casing unbound dump providers, but I do buy the point about locking and I think this is a sensible alternative. At least |UnregisterDumpProviderAsync| taking a scoped ptr makes it clear that it takes ownership of the dump provider. A few minor comments: https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:256: subtle::NoBarrier_Load(&memory_tracing_enabled_)) { On 2015/12/22 15:04:46, Primiano Tucci (OOO til Jan 4) wrote: > ah this logic is wrong. > The else below should be: > else if (take_ownership) ... > > or something equivalent Indeed, disabling tracing should not cause the dumpers to be deleted, should it? https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:178: // Typically nullptr in nominal conditions. /s/nominal/normal/? https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:899: .WillOnce(Invoke([&dtor_count]() -> void { dtor_count++; })); The |-> void| part can be omitted. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:905: mdm_->UnregisterAndDeleteDumpProviderAsync(std::move(mdps.back())); So actually deletion is happening synchronously here? https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:933: base::Passed(scoped_ptr<MemoryDumpProvider>(std::move(mdp))))); Why is the extra |scoped_ptr| temporary required? |mdp| is already a |scoped_ptr|. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:942: .WillOnce(Invoke([&thread_ref]() -> void { The |-> void| part can be omitted. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:956: ASSERT_FALSE(mdp); I would say |ASSERT_NE(nullptr, mdp.get())|, |mdp| is not a boolean. Worse though, this is accessing a moved-from object. A moved-from object is generally in an unspecified state. In this case it works, but this relies on an implementation detail of |scoped_ptr| (it zeroing its pointer when it is moved from). I think you can assume |scoped_ptr| to work properly so this line can just be removed, there already is an |EXPECT_CALL| for the destructor.
Thanks for the review. Took me a while to figure out the sense of this CL post-vacations. Mind taking another look? https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:418: LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, " On 2015/12/22 15:50:09, ssid wrote: > Isn't logging inside lock slow? Done. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:419: << "possibly due to sandboxing (crbug.com/461788)." On 2015/12/22 15:50:09, ssid wrote: > Do you think I should remove this line in my CL, or at least change the log > statement. At that point this if conditions can be merged. and log can be made > common, something like: > "MemoryDumpProvider %s is disabled due to multiple failures or missing thread" Done here. and solved the lock issue https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:426: // The MDP might have been unregistered while we were hopping threads. On 2015/12/22 15:50:09, ssid wrote: > I think this comment is not needed. Right. It's already described in the comment block above. > Also the else condition is not needed. > I think the code should be: > > if (consecutive_failures >= kMaxConsecutiveFailuresCount or post_task_failed) { > mdpinfo->disabled = true; > LOG(ERROR) << "MemoryDumpProvider %s is disabled due to multiple failures or > missing thread"; > } > skip_dump = mdpinfo->disabled; > > But if you are keen on adding log only for that case, you could have 2 > conditions. else is still not needed. Switched to the 2 conditions. Done. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:431: if (!skip_dump) { On 2015/12/22 15:50:09, ssid wrote: > I wonder why shouldn't this be should_dump or similarly positive. You are > checking for ! of negative here. I get confused every time i read this code. Right, with the current logic should_dump makes more sense. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:178: // Typically nullptr in nominal conditions. On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > /s/nominal/normal/? Done. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:899: .WillOnce(Invoke([&dtor_count]() -> void { dtor_count++; })); On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > The |-> void| part can be omitted. Did I already say this week that I hate C++11 lambda syntax? Done https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:905: mdm_->UnregisterAndDeleteDumpProviderAsync(std::move(mdps.back())); On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > So actually deletion is happening synchronously here? Ok, Ok. I see what you mean. I renamed the method to UnregisterAndDeleteSoonDumpProvider https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:913: // from another thread while dumping. It register one MDP and when On 2015/12/22 16:14:56, ssid wrote: > s/register/registers/ Done. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:933: base::Passed(scoped_ptr<MemoryDumpProvider>(std::move(mdp))))); On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > Why is the extra |scoped_ptr| temporary required? |mdp| is already a > |scoped_ptr|. I know but this is for upcasting from scoped_ptr<MockMDP> to scoped_ptr<MDP>. The template magic doesn't align properly in this case with base::Passed and doesn't hit automatically the cast operator. Not sure if there is a better way to achieve this. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:942: .WillOnce(Invoke([&thread_ref]() -> void { On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > The |-> void| part can be omitted. Done. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:956: ASSERT_FALSE(mdp); On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > I would say |ASSERT_NE(nullptr, mdp.get())|, |mdp| is not a boolean. > > Worse though, this is accessing a moved-from object. A moved-from object is > generally in an unspecified state. In this case it works, but this relies on an > implementation detail of |scoped_ptr| (it zeroing its pointer when it is moved > from). > > I think you can assume |scoped_ptr| to work properly so this line can just be > removed, there already is an |EXPECT_CALL| for the destructor. I think you are right. What I am really doing here in this line is testing Passed(scoped_ptr). but I shouldn't be doing this (At least not in this unittest)
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the solution introducing a UnregisterAndDeleteDumpProviderAsync() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the solution introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ==========
A few more nits: > This CL solves the solution introducing a There is a typo in the commit message. https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1430073002/diff/80001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:933: base::Passed(scoped_ptr<MemoryDumpProvider>(std::move(mdp))))); On 2016/01/04 14:44:38, Primiano Tucci wrote: > On 2015/12/29 11:32:46, Ruud van Asseldonk wrote: > > Why is the extra |scoped_ptr| temporary required? |mdp| is already a > > |scoped_ptr|. > > I know but this is for upcasting from scoped_ptr<MockMDP> to scoped_ptr<MDP>. > The template magic doesn't align properly in this case with base::Passed and > doesn't hit automatically the cast operator. > Not sure if there is a better way to achieve this. Ah, I see. https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:252: } else if (subtle::NoBarrier_Load(&memory_tracing_enabled_)) { Can we just get rid of the if here and unconditionally have the DCHECKs below if we don’t take ownership? The check for |memory_tracing_enabled| is nobarrier, so its value should be taken as a hint at best. https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:94: void UnregisterAndDeleteSoonDumpProvider(scoped_ptr<MemoryDumpProvider> mdp); I would call it |UnregisterAndDeleteDumpProviderSoon|, |DeleteSoonDumpProvider| sounds backwards.
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the solution introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the problem introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ==========
Done. Moved the rest (stricter DCHECK) to https://codereview.chromium.org/1559023002 https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:252: } else if (subtle::NoBarrier_Load(&memory_tracing_enabled_)) { On 2016/01/04 15:43:01, Ruud van Asseldonk wrote: > Can we just get rid of the if here and unconditionally have the DCHECKs below if > we don’t take ownership? The check for |memory_tracing_enabled| is nobarrier, so > its value should be taken as a hint at best. Ok, as discussed offline this makes sense. I'm just doing this in a separate CL (crrev.com/1559023002) as there is a chance something might be relying on that and the CL could be reverted. I don't want to lose this CL in that case.
lgtm with one concern and if ruuda@ is happy. https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:429: LOG(ERROR) << "Disabling MemoryDumpProvider \"" << mdpinfo->name << "\". " Doesn't this keep logging messages every dump even if the provider was disabled long back? if the "mdpinfo->consecutive_failures" is greater than 3 for all future dumps or if the thread is gone and we keep trying to post task.
On 2016/01/04 16:39:34, ssid wrote: > lgtm with one concern and if ruuda@ is happy. > > https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... > File base/trace_event/memory_dump_manager.cc (right): > > https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:429: LOG(ERROR) << "Disabling > MemoryDumpProvider \"" << mdpinfo->name << "\". " > Doesn't this keep logging messages every dump even if the provider was disabled > long back? if the "mdpinfo->consecutive_failures" is greater than 3 for all > future dumps or if the thread is gone and we keep trying to post task. LGTM
https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1430073002/diff/120001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:429: LOG(ERROR) << "Disabling MemoryDumpProvider \"" << mdpinfo->name << "\". " On 2016/01/04 16:39:34, ssid wrote: > Doesn't this keep logging messages every dump even if the provider was disabled > long back? if the "mdpinfo->consecutive_failures" is greater than 3 for all > future dumps or if the thread is gone and we keep trying to post task. Oh, good catch. You are definitely right. Look at the new patchset, fixed now.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ruuda@google.com, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/1430073002/#ps140001 (title: "avoid repeating messages")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1430073002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1430073002/140001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the problem introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the problem introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the problem introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 ========== to ========== [tracing] Allow asynchronous unregistration of unbound dump providers Before this change, it was impossible to unregister unbound (i.e. no task runner affinity) dump providers. The reason is that unbound dump providers are invoked on a utility thread, which is transparent to the clients. Therefore the client cannot possibly guarantee that the unregistration will not race with the OnMemoryDump call. This CL solves the problem introducing a UnregisterAndDeleteSoonDumpProvider() method. The method takes ownership of the dump provider and guarantees that the dump provider will be deleted at some point in the near future (even immediately) and its deletion will be guaranteed to not overlap with the OnMemoryDump() call. BUG=461788 Committed: https://crrev.com/4c70669ee6f7bc1fccb61b21b89aa0c83d5c40a5 Cr-Commit-Position: refs/heads/master@{#367505} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4c70669ee6f7bc1fccb61b21b89aa0c83d5c40a5 Cr-Commit-Position: refs/heads/master@{#367505} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
