Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1618703004:
[MemoryInfra] Support dump providers running on SequencedTaskRunner (Closed)
|
Created:
4 years, 11 months ago by ssid Modified:
4 years, 9 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[MemoryInfra] Support dump providers running on SequencedTaskRunner
Some components in chrome require to be on a SequencedTaskRunner while
accessing memory information. This CL adds support for registration and
taking a memory dump on a specified SequencedTaskRunner.
- Support registration using SequencedTaskRunner. Keep the old
register methods for legacy reasons, and since we can't overload
RegisterDumpProvider with a base class parameter, introduce new api
for registration.
- Add a bool in Options to specify if the dump provider uses
SingleThreadtaskrunner, which is usually the case.
- Memory dumps happen on specified SequencedTaskRunner. Keep the old
optimization of grouping dump providers by task runner and do not
make thread hops if registered with SingleThreadTaskRunner and on
right thread.
- Ensure Unregistration should happen on the task runner as well. So,
it is still guaranteed that OnMemoryDump and UnregisterDumpProvider
calls happen on same sequenced task runner, which guarantees no
overlap of tasks.
BUG=555645
TBR=oysteine@chromium.org
Committed: https://crrev.com/7542f07f67d1e614024e6f4f7ef08fe7dfe70a18
Cr-Commit-Position: refs/heads/master@{#377632}
Patch Set 1 #Patch Set 2 : Unittests #Patch Set 3 : Nits. #Patch Set 4 : Split ContinueAsyncProcessDump. #
Total comments: 28
Patch Set 5 : Fixes. #Patch Set 6 : nits. #
Total comments: 18
Patch Set 7 : Fixes. #
Dependent Patchsets: Messages
Total messages: 35 (16 generated)
Description was changed from ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accesing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimisation of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 ========== to ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. ==========
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:100001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks
Description was changed from ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. ========== to ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 ==========
ping
On 2016/01/27 19:49:45, ssid wrote: > ping sorry super slow this week. Will try to catch everything up tomorrow
split the function as discussed offline. PTAL, thanks.
On 2016/02/01 13:21:17, ssid wrote: > split the function as discussed offline. PTAL, thanks. Ping.
Ok this is a rather large change ^__^ Generally makes sense, I just wished we could make this a bit easier to read. See comments inline https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:365: // PostTask is always required for SequencedTaskRunner with no additional s/SequencedTaskRunner with no additional guarantees/generic SequencedTaskRunner/ https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:367: // SetupNextMemoryDump, InvokeOnMemoryDump and FinalizeCurrentDump are called in add () after function names, e.g., SetupNextMemoryDump(), InvokeOnMemoryDump()... https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:378: // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs I think you still need to keep this chunk first, to avoid surprises in the case of an empty dump. Move lines 378-381 before the initial if. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:400: FinalizeCurrentDump(std::move(pmd_async_state)); micro-nit. for consistency with above I'd do the call and return in one line, i.e return FinalizeCurrentDump... this also saves one precious line :) https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:408: // required if we are on the right thread. So invoke dump on the current I'd shorten this saying just: // If |dumps_on_single_thread_task_runner| is true then no PostTask is // required as we are already on the right thread. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:411: return; ditto return Invoke... https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:485: "MemoryDumpManager::SetupNextMemoryDump", Shouldn't this be InvokeOnMemoryDump? https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:504: Hmm this function: - Has a misleading name, especially considering that now we have both FinalizeCurrentDump and FinalizeDumpAndAddToTrace, but they now refer to two different finalization stages. - Doesn't seem to do anything more than the epilogue of SetupNextMemoryDump. Why don't we make the sequence just SetupNextMemoryDump -> InvokeOnMemoryDump -> SetupNextMemoryDump ... eventually goes to FinalizeAndAddToTrace at the end. which feels a bit more easier to follow in my mind https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:75: // |task_runner|, otherwise if passed null |mdp| should be able to handle I'd make this comment a bit simpler and skip the part "RegisterDumpProviderWithSequencedTaskRunner does not accept null" should be kind of obvious and if not, they will hit a dcheck. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:261: // task runners outside of the lock_ to avoid races when disabling tracing. keep this as thread, task_runner doesn't add a lot and makes the sentence less readable https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:298: // Invokes OnMemoryDump of the last MDP and calls FinalizeCurrentDump. Should remove "of the last MDP" it's really misleading. I know what you mean here, but without context it feels like you use this function only once at the end, which is not true. Also "and calls FinalizeCurrentDump" add "at the end" https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:65: void RegisterDumpProviderWithSequencedTaskRunner( this seems a copy-paste of the above. Maybe you want to have a common RegisterDumpProvider which taks a SeqTaskRun as arugment and a boolean (register_seq_task_runn) and have a wrapping function to call the right version https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:125: class TestSequencedTaskRunner : public SequencedTaskRunner { why do you need to create your own SeqTaskRunner? Can't you use an existing one? https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:450: std::vector<scoped_ptr<MockMemoryDumpProvider>> thread_mdps; I'd call them threaded_mdps and sequenced_mdps
Made changes. I couldn't think of better comments, will be happy to get suggestions. PTAL, thanks. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:365: // PostTask is always required for SequencedTaskRunner with no additional On 2016/02/08 12:03:15, Primiano Tucci wrote: > s/SequencedTaskRunner with no additional guarantees/generic SequencedTaskRunner/ Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:367: // SetupNextMemoryDump, InvokeOnMemoryDump and FinalizeCurrentDump are called in On 2016/02/08 12:03:15, Primiano Tucci wrote: > add () after function names, e.g., SetupNextMemoryDump(), > InvokeOnMemoryDump()... Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:378: // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs On 2016/02/08 12:03:15, Primiano Tucci wrote: > I think you still need to keep this chunk first, to avoid surprises in the case > of an empty dump. Move lines 378-381 before the initial if. Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:400: FinalizeCurrentDump(std::move(pmd_async_state)); On 2016/02/08 12:03:15, Primiano Tucci wrote: > micro-nit. for consistency with above I'd do the call and return in one line, > i.e return FinalizeCurrentDump... > this also saves one precious line :) Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:408: // required if we are on the right thread. So invoke dump on the current On 2016/02/08 12:03:15, Primiano Tucci wrote: > I'd shorten this saying just: > > // If |dumps_on_single_thread_task_runner| is true then no PostTask is > // required as we are already on the right thread. Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:411: return; On 2016/02/08 12:03:15, Primiano Tucci wrote: > ditto return Invoke... Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:485: "MemoryDumpManager::SetupNextMemoryDump", On 2016/02/08 12:03:15, Primiano Tucci wrote: > Shouldn't this be InvokeOnMemoryDump? Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:504: On 2016/02/08 12:03:15, Primiano Tucci wrote: > Hmm this function: > - Has a misleading name, especially considering that now we have both > FinalizeCurrentDump and FinalizeDumpAndAddToTrace, but they now refer to two > different finalization stages. > - Doesn't seem to do anything more than the epilogue of SetupNextMemoryDump. > > Why don't we make the sequence just > SetupNextMemoryDump -> InvokeOnMemoryDump -> SetupNextMemoryDump ... eventually > goes to FinalizeAndAddToTrace at the end. > > which feels a bit more easier to follow in my mind > I did not want multiple functions destroying mdpinfo. It's little hard to guess where all MDP is destroyed. Also I had to copy these 2 lines at few places. That is why made a function. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:75: // |task_runner|, otherwise if passed null |mdp| should be able to handle On 2016/02/08 12:03:15, Primiano Tucci wrote: > I'd make this comment a bit simpler and skip the part > "RegisterDumpProviderWithSequencedTaskRunner does not accept null" > should be kind of obvious and if not, they will hit a dcheck. Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:261: // task runners outside of the lock_ to avoid races when disabling tracing. On 2016/02/08 12:03:15, Primiano Tucci wrote: > keep this as thread, task_runner doesn't add a lot and makes the sentence less > readable Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:298: // Invokes OnMemoryDump of the last MDP and calls FinalizeCurrentDump. Should On 2016/02/08 12:03:15, Primiano Tucci wrote: > remove "of the last MDP" it's really misleading. > I know what you mean here, but without context it feels like you use this > function only once at the end, which is not true. > > Also "and calls FinalizeCurrentDump" add "at the end" Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:65: void RegisterDumpProviderWithSequencedTaskRunner( On 2016/02/08 12:03:15, Primiano Tucci wrote: > this seems a copy-paste of the above. > Maybe you want to have a common RegisterDumpProvider which taks a SeqTaskRun as > arugment and a boolean (register_seq_task_runn) and have a wrapping function to > call the right version Done. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:125: class TestSequencedTaskRunner : public SequencedTaskRunner { On 2016/02/08 12:03:15, Primiano Tucci wrote: > why do you need to create your own SeqTaskRunner? Can't you use an existing one? There seems to be no SeqTaskRunner implementation in base. I am using the SequencedWorkerPool with single token to make a SeqTaskRunner. https://codereview.chromium.org/1618703004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:450: std::vector<scoped_ptr<MockMemoryDumpProvider>> thread_mdps; On 2016/02/08 12:03:15, Primiano Tucci wrote: > I'd call them threaded_mdps and sequenced_mdps Done.
Patchset #7 (id:200001) has been deleted
Patchset #5 (id:160001) has been deleted
ping.
On 2016/02/16 20:19:01, ssid wrote: > ping. apologies was sheriffing these two days. will do as first thing tomorrow morning (As well as starting the doc as I promised)
Thanks a lot ssid. The code looks good (with some minimal comment) I think I have just some bigger comment / question left on the test. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:440: // PostTask failed. Nack current dump and continue. I think you should remove the "Nack current dump" part. IIRC we have seen this happening in the past with gpu threads having short lifespan and we decided that we should just keep working (without NACKing) in this case. To be clear, I think the code is already fine, just the comment is wrong. Does it make sense or am I missing somethinG? https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:62: reinterpret_cast<const scoped_refptr<base::SingleThreadTaskRunner>&>( Hmm this would be a long conversation but TL;DR I don't think that upcasting scoped_refptr is guaranteed to be safe. Let's just be safe, and do an explicit copy here, i.e.: scoped_refptr<base::SingleThreadTaskRunner> single_thread_task_runner = static_cast<base::SingleThreadTaskRunner*> task_runner.get(); https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:146: disabled_(false), why calling it with negative names? Just call it enabled_(true) :) https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:149: void set_enabled() { disabled_ = false; } just set_enabled(bool value) { enabled_ = value; } ? https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:163: EXPECT_EQ(TimeDelta(), delay); not sure why you are asserting this. At most this is a DCHECK not a test expectation. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:180: unsigned no_of_post_tasks_; s/so/num/ https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:465: std::vector<scoped_refptr<TestSequencedTaskRunner>> sequenced_task_runners; hmm I would keep the tests separate otherwise this TEST_F will be impossible to debug. Since you are adding coverage below, I would leave this test as it is https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:577: // Tasks should be posted even if |mdps[1]| and |mdps[2]| belong to same task s/posted/individually posted/ https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:589: EXPECT_EQ(2u, task_runner1->no_of_post_tasks()); Hmm this I don't follow: how does task_runner1 get 2 post task? According to your comment shouldn't it get 0?
Removed changes from the existing test. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:440: // PostTask failed. Nack current dump and continue. On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > I think you should remove the "Nack current dump" part. > IIRC we have seen this happening in the past with gpu threads having short > lifespan and we decided that we should just keep working (without NACKing) in > this case. > To be clear, I think the code is already fine, just the comment is wrong. > Does it make sense or am I missing somethinG? Sorry. I mixed up between dump provider dump and PMD. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:62: reinterpret_cast<const scoped_refptr<base::SingleThreadTaskRunner>&>( On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > Hmm this would be a long conversation but TL;DR I don't think that upcasting > scoped_refptr is guaranteed to be safe. > Let's just be safe, and do an explicit copy here, i.e.: > > scoped_refptr<base::SingleThreadTaskRunner> single_thread_task_runner = > static_cast<base::SingleThreadTaskRunner*> task_runner.get(); Done. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:146: disabled_(false), On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > why calling it with negative names? Just call it enabled_(true) :) fixed. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:149: void set_enabled() { disabled_ = false; } On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > just set_enabled(bool value) { enabled_ = value; } ? Done. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:163: EXPECT_EQ(TimeDelta(), delay); On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > not sure why you are asserting this. > At most this is a DCHECK not a test expectation. yeah it's not required. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:180: unsigned no_of_post_tasks_; On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > s/so/num/ Done. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:465: std::vector<scoped_refptr<TestSequencedTaskRunner>> sequenced_task_runners; On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > hmm I would keep the tests separate otherwise this TEST_F will be impossible to > debug. > Since you are adding coverage below, I would leave this test as it is Done. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:577: // Tasks should be posted even if |mdps[1]| and |mdps[2]| belong to same task On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > s/posted/individually posted/ Done. https://codereview.chromium.org/1618703004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager_unittest.cc:589: EXPECT_EQ(2u, task_runner1->no_of_post_tasks()); On 2016/02/18 16:14:37, Primiano (throttled til Mar 4) wrote: > Hmm this I don't follow: how does task_runner1 get 2 post task? According to > your comment shouldn't it get 0? The comment is at wrong place. The post task happens even for disabled provider, but OnMemoryDump is not called. Fixed the comment.
LGTM thanks!
The CQ bit was checked by primiano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618703004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618703004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ssid@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine for one line change in components/tracing. PTAL, thanks
On 2016/02/25 18:18:47, ssid wrote: > +oysteine for one line change in components/tracing. > PTAL, thanks considering that it's a trivial one line change there I think it's fine to just TBR
Description was changed from ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 ========== to ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 TBR=oysteine@chromium.org ==========
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618703004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618703004/240001
Message was sent while issue was closed.
Description was changed from ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 TBR=oysteine@chromium.org ========== to ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 TBR=oysteine@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 TBR=oysteine@chromium.org ========== to ========== [MemoryInfra] Support dump providers running on SequencedTaskRunner Some components in chrome require to be on a SequencedTaskRunner while accessing memory information. This CL adds support for registration and taking a memory dump on a specified SequencedTaskRunner. - Support registration using SequencedTaskRunner. Keep the old register methods for legacy reasons, and since we can't overload RegisterDumpProvider with a base class parameter, introduce new api for registration. - Add a bool in Options to specify if the dump provider uses SingleThreadtaskrunner, which is usually the case. - Memory dumps happen on specified SequencedTaskRunner. Keep the old optimization of grouping dump providers by task runner and do not make thread hops if registered with SingleThreadTaskRunner and on right thread. - Ensure Unregistration should happen on the task runner as well. So, it is still guaranteed that OnMemoryDump and UnregisterDumpProvider calls happen on same sequenced task runner, which guarantees no overlap of tasks. BUG=555645 TBR=oysteine@chromium.org Committed: https://crrev.com/7542f07f67d1e614024e6f4f7ef08fe7dfe70a18 Cr-Commit-Position: refs/heads/master@{#377632} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7542f07f67d1e614024e6f4f7ef08fe7dfe70a18 Cr-Commit-Position: refs/heads/master@{#377632} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
