Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067793004:
[memory-infra] Add support for queueing memory dump requests in the browser process (Closed)
|
Created:
4 years, 6 months ago by petrcermak Modified:
4 years, 6 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, darin-cc_chromium.org, jam, wfh+watch_chromium.org, mythria, kraynov Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[memory-infra] Add support for queueing memory dump requests in the browser process
This patch changes TracingControllerImpl to queue global memory dump
requests if another dump is in progress (instead of rejecting the
requests). The requests are processed in a simple FIFO order.
To avoid choking the controller, periodic dump requests are skipped if
the queue already contains a (possibly non-periodic) dump request with
the same level of detail.
Rationale: This is necessary for Telemetry tests which combine periodic
and triggered memory dumps.
Note: Support for queueing global memory dump requests in child
processes will be added in a separate patch.
BUG=619935
Committed: https://crrev.com/2868e10408e7fc7d3dba27e813e0ff237869bdee
Cr-Commit-Position: refs/heads/master@{#401865}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressed review comments #
Total comments: 18
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address final comments #
Messages
Total messages: 28 (10 generated)
petrcermak@chromium.org changed reviewers: + primiano@chromium.org
PTAL. I have to admit it's been quite a while since I wrote C++ code :-\ I stress-tested this against Telemetry. To try it yourself, do the following (assuming you have built Chrome in out/Release): $ (cd third_party/catapult/ && (git cl patch 2065233003; git cl issue 0)) $ third_party/catapult/telemetry/bin/run_tests testDumpQueueing --browser=release Thanks, Petr https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:1050: if (!enqueued_memory_dump_requests_.empty()) { Maybe it would be better to post a task instead?
petrcermak@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine (owner)
ssid@chromium.org changed reviewers: + ssid@chromium.org
Few thoughts about this change. Even if we don't agree on all points i would like the periodic dumps behavior to remain as is. Thanks, Sid. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); For the case of only periodic dumps, if a heavy dump gets stalled for 2 seconds, then we will accumulate 7 light dumps on the queue. This would start triggering 7 light dumps in a row and then come to the heavy dumps. If the page was loading for 10 seconds, then we would see doing too many queued memory dumps at the end which are not really useful to see. Currently we would start making periodic dumps once the page loads finishes. Is there a reason we should queue more than one dump? Is there a reason to queue a light dump followed by a heavy dump? I think 1. We should queue memory dumps only when it is explicitly_triggered 2. This queue should have a maximum size of 1. 3. The logic could be if more request comes by, and if the request is heavier than the current dump only then queue it. I mean: if we are currently doing a heavy dump and a light dump request comes, there is no need to queue it since we already have those details in the current dump. But if we are doing a heavy dump, and a new heavy dump is requested then it should be queued because V8 gc might have collected different set of stats after the dump provider was called. essentially what i am trying to say is the code should look like: if (pending_memory_dump_guid_ && current_level_of_detail < requested_level_of_detail) { if (enqueued_memory_dump_request_ && enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) { enqueued_memory_dump_request_ = new_request; } } This also solves the current problem of heavy dumps going missing if the light dump gets stalled. ie, if the 7th light dump takes too long, the heavy dump might go missing because it gets ignored. This would ensure that the heavy dump surely happens and the light dumps are unnecessary to see there.
Some initial comments, thanks for the work! https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:232: base::RunLoop loop1; never seen this pattern, why do you need 4 run loops?. I think you want four scopes { RunLoop loop; .... loop.run() } Also I think you don't need all this cmplexity. As long as you have one MDP bound to the same thread, that will necessarily prevent the dump to complete before you finish the current task (because it depends on task posted next) https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:239: base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC, I think you don't need this waitable event if you just have one MDP bound to the same thread of this (i.e. ThreadTaskRunnerHandle::Get() https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:925: if (pending_memory_dump_guid_) { I think here I'd just if (!queued_memory_dump_requests.empty()) so there is only one state (the queue) to care about https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); On 2016/06/15 17:20:51, ssid wrote: > For the case of only periodic dumps, if a heavy dump gets stalled for 2 seconds, > then we will accumulate 7 light dumps on the queue. This would start triggering > 7 light dumps in a row and then come to the heavy dumps. If the page was loading > for 10 seconds, then we would see doing too many queued memory dumps at the end > which are not really useful to see. > Currently we would start making periodic dumps once the page loads finishes. > > Is there a reason we should queue more than one dump? > Is there a reason to queue a light dump followed by a heavy dump? > > I think > 1. We should queue memory dumps only when it is explicitly_triggered > 2. This queue should have a maximum size of 1. > 3. The logic could be if more request comes by, and if the request is heavier > than the current dump only then queue it. I mean: if we are currently doing a > heavy dump and a light dump request comes, there is no need to queue it since we > already have those details in the current dump. But if we are doing a heavy > dump, and a new heavy dump is requested then it should be queued because V8 gc > might have collected different set of stats after the dump provider was called. > > essentially what i am trying to say is the code should look like: > if (pending_memory_dump_guid_ && current_level_of_detail < > requested_level_of_detail) { > if (enqueued_memory_dump_request_ && > enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) { > enqueued_memory_dump_request_ = new_request; > } > } > > This also solves the current problem of heavy dumps going missing if the light > dump gets stalled. ie, if the 7th light dump takes too long, the heavy dump > might go missing because it gets ignored. This would ensure that the heavy dump > surely happens and the light dumps are unnecessary to see there. So, I agree with ssid that this can cause some storms if too many light dumps get queued becuase the message loops are busy. However, I'd simplify A bit the proposal. What I'd do personally is: always enqueue the dump request, unless: the request is of type PERIODIC_INTERVAL and the level of detail matches the top of the queue. (in essence, deduplicate and don't queue identical periodic requests) WDYT? https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:934: const base::trace_event::MemoryDumpRequestArgs& args, Does this require any arg? Aren't this the front() of the queue? https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:1050: if (!enqueued_memory_dump_requests_.empty()) { On 2016/06/15 16:19:11, petrcermak wrote: > Maybe it would be better to post a task instead? Yeah that will avoid thinking about weird re-entrancy in the rare cases where you have no child processes. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.h (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:86: struct GlobalMemoryDumpRequest { I'd call this QueuedMemoryDumpRequest, at first I was a bit confused why we have this struct here. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:205: uint64_t pending_memory_dump_guid_; I think at this point you can get rid of pending_guid and pending_callback as they will be just the guid and the callback of the front() of the queue. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:207: std::queue<GlobalMemoryDumpRequest> enqueued_memory_dump_requests_; I'd drop the "en". I'm not a native speaker but "enqueued_something" sounds a bit odd. cs shows 6 results for \benqueued_\w+ and 135 for \bqueued_\w+ :)
Description was changed from ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 ========== to ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. To avoid choking the controller, periodic dump requests are skipped if the queue already contains a (possibly non-periodic) dump request with the same level of detail. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 ==========
Thanks for your comment's. PTAL. Thanks, Petr https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:232: base::RunLoop loop1; On 2016/06/16 08:37:23, Primiano Tucci wrote: > never seen this pattern, why do you need 4 run loops?. > I think you want four scopes { RunLoop loop; .... loop.run() } > > Also I think you don't need all this cmplexity. As long as you have one MDP > bound to the same thread, that will necessarily prevent the dump to complete > before you finish the current task (because it depends on task posted next) The problem is that I need to do the following: REQUEST1 ---------------------------> FINISHED1 REQUEST2 -----------------------------> FINISHED2 ^ : Because I'm interested in this ..: Therefore, the run loops need to overlap. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:239: base::WaitableEvent event(base::WaitableEvent::ResetPolicy::AUTOMATIC, On 2016/06/16 08:37:23, Primiano Tucci wrote: > I think you don't need this waitable event if you just have one MDP bound to the > same thread of this (i.e. ThreadTaskRunnerHandle::Get() OK. Just to check: Am I guaranteed that a dump doesn't finish (from TraceController's point of view) before RequestGlobalMemoryDump exits? https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:925: if (pending_memory_dump_guid_) { On 2016/06/16 08:37:23, Primiano Tucci wrote: > I think here I'd just if (!queued_memory_dump_requests.empty()) > > so there is only one state (the queue) to care about Done. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); On 2016/06/16 08:37:23, Primiano Tucci wrote: > On 2016/06/15 17:20:51, ssid wrote: > > For the case of only periodic dumps, if a heavy dump gets stalled for 2 > seconds, > > then we will accumulate 7 light dumps on the queue. This would start > triggering > > 7 light dumps in a row and then come to the heavy dumps. If the page was > loading > > for 10 seconds, then we would see doing too many queued memory dumps at the > end > > which are not really useful to see. > > Currently we would start making periodic dumps once the page loads finishes. > > > > Is there a reason we should queue more than one dump? > > Is there a reason to queue a light dump followed by a heavy dump? > > > > I think > > 1. We should queue memory dumps only when it is explicitly_triggered > > 2. This queue should have a maximum size of 1. > > 3. The logic could be if more request comes by, and if the request is heavier > > than the current dump only then queue it. I mean: if we are currently doing a > > heavy dump and a light dump request comes, there is no need to queue it since > we > > already have those details in the current dump. But if we are doing a heavy > > dump, and a new heavy dump is requested then it should be queued because V8 gc > > might have collected different set of stats after the dump provider was > called. > > > > essentially what i am trying to say is the code should look like: > > if (pending_memory_dump_guid_ && current_level_of_detail < > > requested_level_of_detail) { > > if (enqueued_memory_dump_request_ && > > enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) { > > enqueued_memory_dump_request_ = new_request; > > } > > } > > > > This also solves the current problem of heavy dumps going missing if the light > > dump gets stalled. ie, if the 7th light dump takes too long, the heavy dump > > might go missing because it gets ignored. This would ensure that the heavy > dump > > surely happens and the light dumps are unnecessary to see there. > > So, I agree with ssid that this can cause some storms if too many light dumps > get queued becuase the message loops are busy. However, I'd simplify A bit the > proposal. > What I'd do personally is: > always enqueue the dump request, unless: the request is of type > PERIODIC_INTERVAL and the level of detail matches the top of the queue. (in > essence, deduplicate and don't queue identical periodic requests) > WDYT? I think that the following would be the best approach: If the following conditions both hold when requesting a dump, there's no point in queueing the request: 1. The request is for a periodic dump. 2. There is another dump in the queue (potentially being performed at the moment) which has the same level of detail. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:934: const base::trace_event::MemoryDumpRequestArgs& args, On 2016/06/16 08:37:23, Primiano Tucci wrote: > Does this require any arg? Aren't this the front() of the queue? They were not, but I changed it, so done. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:1050: if (!enqueued_memory_dump_requests_.empty()) { On 2016/06/16 08:37:23, Primiano Tucci wrote: > On 2016/06/15 16:19:11, petrcermak wrote: > > Maybe it would be better to post a task instead? > > Yeah that will avoid thinking about weird re-entrancy in the rare cases where > you have no child processes. Done. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.h (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:86: struct GlobalMemoryDumpRequest { On 2016/06/16 08:37:23, Primiano Tucci wrote: > I'd call this QueuedMemoryDumpRequest, at first I was a bit confused why we have > this struct here. Done. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:205: uint64_t pending_memory_dump_guid_; On 2016/06/16 08:37:23, Primiano Tucci wrote: > I think at this point you can get rid of pending_guid and pending_callback as > they will be just the guid and the callback of the front() of the queue. Done. https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.h:207: std::queue<GlobalMemoryDumpRequest> enqueued_memory_dump_requests_; On 2016/06/16 08:37:23, Primiano Tucci wrote: > I'd drop the "en". I'm not a native speaker but "enqueued_something" sounds a > bit odd. > cs shows 6 results for \benqueued_\w+ and 135 for \bqueued_\w+ :) Done, but "enqueued" sounded more posh :-P
https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); On 2016/06/16 12:08:31, petrcermak wrote: > On 2016/06/16 08:37:23, Primiano Tucci wrote: > > On 2016/06/15 17:20:51, ssid wrote: > > > For the case of only periodic dumps, if a heavy dump gets stalled for 2 > > seconds, > > > then we will accumulate 7 light dumps on the queue. This would start > > triggering > > > 7 light dumps in a row and then come to the heavy dumps. If the page was > > loading > > > for 10 seconds, then we would see doing too many queued memory dumps at the > > end > > > which are not really useful to see. > > > Currently we would start making periodic dumps once the page loads finishes. > > > > > > Is there a reason we should queue more than one dump? > > > Is there a reason to queue a light dump followed by a heavy dump? > > > > > > I think > > > 1. We should queue memory dumps only when it is explicitly_triggered > > > 2. This queue should have a maximum size of 1. > > > 3. The logic could be if more request comes by, and if the request is > heavier > > > than the current dump only then queue it. I mean: if we are currently doing > a > > > heavy dump and a light dump request comes, there is no need to queue it > since > > we > > > already have those details in the current dump. But if we are doing a heavy > > > dump, and a new heavy dump is requested then it should be queued because V8 > gc > > > might have collected different set of stats after the dump provider was > > called. > > > > > > essentially what i am trying to say is the code should look like: > > > if (pending_memory_dump_guid_ && current_level_of_detail < > > > requested_level_of_detail) { > > > if (enqueued_memory_dump_request_ && > > > enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) { > > > enqueued_memory_dump_request_ = new_request; > > > } > > > } > > > > > > This also solves the current problem of heavy dumps going missing if the > light > > > dump gets stalled. ie, if the 7th light dump takes too long, the heavy dump > > > might go missing because it gets ignored. This would ensure that the heavy > > dump > > > surely happens and the light dumps are unnecessary to see there. > > > > So, I agree with ssid that this can cause some storms if too many light dumps > > get queued becuase the message loops are busy. However, I'd simplify A bit the > > proposal. > > What I'd do personally is: > > always enqueue the dump request, unless: the request is of type > > PERIODIC_INTERVAL and the level of detail matches the top of the queue. (in > > essence, deduplicate and don't queue identical periodic requests) > > WDYT? > > I think that the following would be the best approach: If the following > conditions both hold when requesting a dump, there's no point in queueing the > request: > > 1. The request is for a periodic dump. > 2. There is another dump in the queue (potentially being performed at the > moment) which has the same level of detail. If there is a sequence of events like this when a DETAILED dump is in progress: 1. request: periodic LIGHT dump 2. request: periodic DETAILED dump 3. request: explicit DETAILED dump 4. request: explicit DETAILED dump Is there any use of making a LIGHT dump before actually making a DETAILED dump when the DETAILED dump would anyway include all the details needed for light dump? Is there any reason to still perform a DETAILED dump after previous DETAILED dump, and also multiple times when explicitly called? If the only reason to dump multiple times is for running the success callback, maybe we only need a queue for callbacks to run. And just a single dump in the Queue (the most detailed one requested). We could call the light dump request a success if the heavy dump was successful. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:222: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { I am removing SingleProcessMemoryTracingTest in this CL https://codereview.chromium.org/2047533002/ Is there a reason you need a single process test here? Why won't a multi process test work here?
https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); On 2016/06/16 23:12:03, ssid wrote: > On 2016/06/16 12:08:31, petrcermak wrote: > > On 2016/06/16 08:37:23, Primiano Tucci wrote: > > > On 2016/06/15 17:20:51, ssid wrote: > > > > For the case of only periodic dumps, if a heavy dump gets stalled for 2 > > > seconds, > > > > then we will accumulate 7 light dumps on the queue. This would start > > > triggering > > > > 7 light dumps in a row and then come to the heavy dumps. If the page was > > > loading > > > > for 10 seconds, then we would see doing too many queued memory dumps at > the > > > end > > > > which are not really useful to see. > > > > Currently we would start making periodic dumps once the page loads > finishes. > > > > > > > > Is there a reason we should queue more than one dump? > > > > Is there a reason to queue a light dump followed by a heavy dump? > > > > > > > > I think > > > > 1. We should queue memory dumps only when it is explicitly_triggered > > > > 2. This queue should have a maximum size of 1. > > > > 3. The logic could be if more request comes by, and if the request is > > heavier > > > > than the current dump only then queue it. I mean: if we are currently > doing > > a > > > > heavy dump and a light dump request comes, there is no need to queue it > > since > > > we > > > > already have those details in the current dump. But if we are doing a > heavy > > > > dump, and a new heavy dump is requested then it should be queued because > V8 > > gc > > > > might have collected different set of stats after the dump provider was > > > called. > > > > > > > > essentially what i am trying to say is the code should look like: > > > > if (pending_memory_dump_guid_ && current_level_of_detail < > > > > requested_level_of_detail) { > > > > if (enqueued_memory_dump_request_ && > > > > enqueued_memory_dump_request_.level_of_detail < requested_level_of_detail) > { > > > > enqueued_memory_dump_request_ = new_request; > > > > } > > > > } > > > > > > > > This also solves the current problem of heavy dumps going missing if the > > light > > > > dump gets stalled. ie, if the 7th light dump takes too long, the heavy > dump > > > > might go missing because it gets ignored. This would ensure that the heavy > > > dump > > > > surely happens and the light dumps are unnecessary to see there. > > > > > > So, I agree with ssid that this can cause some storms if too many light > dumps > > > get queued becuase the message loops are busy. However, I'd simplify A bit > the > > > proposal. > > > What I'd do personally is: > > > always enqueue the dump request, unless: the request is of type > > > PERIODIC_INTERVAL and the level of detail matches the top of the queue. (in > > > essence, deduplicate and don't queue identical periodic requests) > > > WDYT? > > > > I think that the following would be the best approach: If the following > > conditions both hold when requesting a dump, there's no point in queueing the > > request: > > > > 1. The request is for a periodic dump. > > 2. There is another dump in the queue (potentially being performed at the > > moment) which has the same level of detail. > > If there is a sequence of events like this when a DETAILED dump is in progress: > 1. request: periodic LIGHT dump > 2. request: periodic DETAILED dump > 3. request: explicit DETAILED dump > 4. request: explicit DETAILED dump > > Is there any use of making a LIGHT dump before actually making a DETAILED dump > when the DETAILED dump would anyway include all the details needed for light > dump? > > Is there any reason to still perform a DETAILED dump after previous DETAILED > dump, and also multiple times when explicitly called? > > If the only reason to dump multiple times is for running the success callback, > maybe we only need a queue for callbacks to run. And just a single dump in the > Queue (the most detailed one requested). > We could call the light dump request a success if the heavy dump was successful. I'm fine with that, BUT that would require re-architecting other parts of the pipeline. The problem is that the TracingControllerImpl is already given a GUID for the dump it's supposed to create. Therefore, you can't call multiple callbacks (after a single dump succeeds) and tell them they were all successful because each of them is expecting a different GUID. Another thing to think about: Sometimes you want a less detailed dump instead of a more detailed one for reasons other than performance. For example, when you request a BACKGROUND dump, you definitely do NOT want it to be a LIGHT or DETAILED dump. Either way, I think that this is something that we can address later. We don't need to have a perfect solution straightaway. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:222: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { On 2016/06/16 23:12:03, ssid wrote: > I am removing SingleProcessMemoryTracingTest in this CL > https://codereview.chromium.org/2047533002/ > > Is there a reason you need a single process test here? Why won't a multi process > test work here? Not that I'm aware of, I just followed what the other tests did.
+cc kraynov
https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_controller_impl.cc:926: enqueued_memory_dump_requests_.emplace(args, callback); > I'm fine with that, BUT that would require re-architecting other parts of the > pipeline. The problem is that the TracingControllerImpl is already given a GUID > for the dump it's supposed to create. Therefore, you can't call multiple > callbacks (after a single dump succeeds) and tell them they were all successful > because each of them is expecting a different GUID. Ah I see, then I am fine with this, if that is going to be a bigger change. I was thinking it is a simple check. After all it these are rare cases anyway. > Another thing to think about: Sometimes you want a less detailed dump instead of > a more detailed one for reasons other than performance. For example, when you > request a BACKGROUND dump, you definitely do NOT want it to be a LIGHT or > DETAILED dump. Agreed. > Either way, I think that this is something that we can address later. We don't > need to have a perfect solution straightaway. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:222: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { On 2016/06/17 08:36:44, petrcermak wrote: > On 2016/06/16 23:12:03, ssid wrote: > > I am removing SingleProcessMemoryTracingTest in this CL > > https://codereview.chromium.org/2047533002/ > > > > Is there a reason you need a single process test here? Why won't a multi > process > > test work here? > > Not that I'm aware of, I just followed what the other tests did. Sorry, I got confused with the test files here. Ignore my comment, this looks good.
https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:222: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { Had a chat offline on this. Let's simplify it Req(PL) true Req(PL) false ReqANdWait(PL) falsew Req(PL) true Req(PL) false ReqANdWait(PL) false -------------- Req(EL) true Req(PL) false ReqAndWait(EL) True ReqAndWait(PL) true https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:926: bool another_dump_already_in_progress = !queued_memory_dump_requests_.empty(); I really love this perfectly touching the 80th col without wrapping :) https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:928: // If this is a periodic memory dump request without a callback and there remove the "without a callback" part. It is not true and if it was true would be wrong. I think realistically most of them have a callback https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:931: if (another_dump_already_in_progress) { why not if (another... && dump_type == ). Would reduce the indentation level of the code below and increase its readability. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:934: const auto requested_level_of_detail = args.level_of_detail; not sure this variable buys you anything really https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:1070: { why this extra scope, seems unnecessary? https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.h (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.h:122: void PerformGlobalMemoryDump(); Maybe PerformNextQueuedGlobalMemoryDump ?
Thanks for your comments! PTAL. Petr https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:222: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { On 2016/06/23 11:09:16, Primiano Tucci wrote: > Had a chat offline on this. Let's simplify it > > Req(PL) true > Req(PL) false > ReqANdWait(PL) falsew > > Req(PL) true > Req(PL) false > ReqANdWait(PL) false > > -------------- > > Req(EL) true > Req(PL) false > ReqAndWait(EL) True > ReqAndWait(PL) true Done (but kept everything in one test). https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:926: bool another_dump_already_in_progress = !queued_memory_dump_requests_.empty(); On 2016/06/23 11:09:16, Primiano Tucci wrote: > I really love this perfectly touching the 80th col without wrapping :) Ack :-) https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:928: // If this is a periodic memory dump request without a callback and there On 2016/06/23 11:09:16, Primiano Tucci wrote: > remove the "without a callback" part. It is not true and if it was true would be > wrong. I think realistically most of them have a callback Done. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:931: if (another_dump_already_in_progress) { On 2016/06/23 11:09:16, Primiano Tucci wrote: > why not if (another... && dump_type == ). Would reduce the indentation level of > the code below and increase its readability. Done. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:934: const auto requested_level_of_detail = args.level_of_detail; On 2016/06/23 11:09:16, Primiano Tucci wrote: > not sure this variable buys you anything really I removed it. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:1070: { On 2016/06/23 11:09:16, Primiano Tucci wrote: > why this extra scope, seems unnecessary? It's because |callback| is a reference to the front of the queue, which is popped on line 1076. If there's no scope, someone could incorrectly use |callback| after it's popped. https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.h (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.h:122: void PerformGlobalMemoryDump(); On 2016/06/23 11:09:17, Primiano Tucci wrote: > Maybe PerformNextQueuedGlobalMemoryDump ? Done.
LGTM thanks https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2067793004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:1070: { On 2016/06/24 10:49:57, petrcermak wrote: > On 2016/06/23 11:09:16, Primiano Tucci wrote: > > why this extra scope, seems unnecessary? > > It's because |callback| is a reference to the front of the queue, which is > popped on line 1076. If there's no scope, someone could incorrectly use > |callback| after it's popped. ahh right. smart. https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:228: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { Plz add a comment here explaining briefly what are you testing, as this is a bit tricky to reconstruct just from the code. https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:274: EXPECT_CALL(*this, OnMemoryDumpDone(0, true /* success */)); why don't you put also these inline? The order shouldn't matter, but is a bit odd that 0 1 and 3 are above, and the rest here.
The CQ bit was checked by petrcermak@chromium.org
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/2067793004/#ps60001 (title: "Address final comments")
Thanks for your comments. Petr https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:228: IN_PROC_BROWSER_TEST_F(SingleProcessMemoryTracingTest, QueuedDumps) { On 2016/06/24 11:23:05, Primiano Tucci wrote: > Plz add a comment here explaining briefly what are you testing, as this is a bit > tricky to reconstruct just from the code. Done. https://codereview.chromium.org/2067793004/diff/40001/content/browser/tracing... content/browser/tracing/memory_tracing_browsertest.cc:274: EXPECT_CALL(*this, OnMemoryDumpDone(0, true /* success */)); On 2016/06/24 11:23:05, Primiano Tucci wrote: > why don't you put also these inline? The order shouldn't matter, but is a bit > odd that 0 1 and 3 are above, and the rest here. Done.
The CQ bit was unchecked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067793004/60001
lgtm!
The CQ bit was checked by petrcermak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. To avoid choking the controller, periodic dump requests are skipped if the queue already contains a (possibly non-periodic) dump request with the same level of detail. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 ========== to ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. To avoid choking the controller, periodic dump requests are skipped if the queue already contains a (possibly non-periodic) dump request with the same level of detail. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. To avoid choking the controller, periodic dump requests are skipped if the queue already contains a (possibly non-periodic) dump request with the same level of detail. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 ========== to ========== [memory-infra] Add support for queueing memory dump requests in the browser process This patch changes TracingControllerImpl to queue global memory dump requests if another dump is in progress (instead of rejecting the requests). The requests are processed in a simple FIFO order. To avoid choking the controller, periodic dump requests are skipped if the queue already contains a (possibly non-periodic) dump request with the same level of detail. Rationale: This is necessary for Telemetry tests which combine periodic and triggered memory dumps. Note: Support for queueing global memory dump requests in child processes will be added in a separate patch. BUG=619935 Committed: https://crrev.com/2868e10408e7fc7d3dba27e813e0ff237869bdee Cr-Commit-Position: refs/heads/master@{#401865} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2868e10408e7fc7d3dba27e813e0ff237869bdee Cr-Commit-Position: refs/heads/master@{#401865} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
