Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815963002:
[memory-infra] Trigger first periodic dump after initial period (Closed)
|
Created:
3 years, 8 months ago by ssid Modified:
3 years, 8 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, tracing+reviews_chromium.org, danakj+watch_chromium.org, chiniforooshan Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[memory-infra] Trigger first periodic dump after initial period
The periodic dumps behavior was to create memory dump after the first
period ended. It is makes startup tracing worse in case we want a
single dump and not the dump at start. So, get back old behavior.
BUG=607533
R=primiano@chromium.org
Review-Url: https://codereview.chromium.org/2815963002
Cr-Commit-Position: refs/heads/master@{#464539}
Committed: https://chromium.googlesource.com/chromium/src/+/1a345de25847354fb1e1b1b81bbd00a64af8d55b
Patch Set 1 #
Total comments: 3
Patch Set 2 : Just fix peridic dumps. #Messages
Total messages: 20 (12 generated)
ptal, thanks!
chiniforooshan@chromium.org changed reviewers: + chiniforooshan@chromium.org
https://codereview.chromium.org/2815963002/diff/1/content/browser/tracing/mem... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2815963002/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:40: if (evt) I know that BrowserInitiatedDump is flaky because the global dump request is sent before tracing is enabled on all processes, but I don't understand why the signalling changes anything. Doesn't the EnableMemoryTracing wait until the quit closure is run?
meh, I'll close this. https://codereview.chromium.org/2815963002/diff/1/content/browser/tracing/mem... File content/browser/tracing/memory_tracing_browsertest.cc (right): https://codereview.chromium.org/2815963002/diff/1/content/browser/tracing/mem... content/browser/tracing/memory_tracing_browsertest.cc:40: if (evt) On 2017/04/12 19:32:57, chiniforooshan wrote: > I know that BrowserInitiatedDump is flaky because the global dump request is > sent before tracing is enabled on all processes, but I don't understand why the > signalling changes anything. Doesn't the EnableMemoryTracing wait until the quit > closure is run? Yes, you are right! I just realized both the tests have runloop till tracing is enabled.
Message was sent while issue was closed.
So, the change to the scheduler LG, but I don't fully understand the change to the test. Both the runloop AND the WaitableEvent seem redundant. Can we just make EnableMemoryTracing() wait until tracing has started? However, it seems that is already the case. By looking at the code it seems that EnableMemoryTracing already waits on the RunLoop. So what is going wrong? https://codereview.chromium.org/2815963002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_scheduler.cc (right): https://codereview.chromium.org/2815963002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_scheduler.cc:75: // Keep the legacy behavior of periodic dumps where the first dump is I'd reword this comment to say: (also add a +blank line before) // For startup tracing we expect the first dump to happen after |period_ms| and not immediatley saying "legacy behavior" is really confusing because the legacy code has already gone :) This code is now the new legacy :P
Message was sent while issue was closed.
Description was changed from ========== Fix memory-infra tests The periodic dumps behavior was to create memory dump after the first period ended. This causes flakiness in browser tests. It is also make startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. The BrowserInitiatedDump test expects memory dump to succeed without waiting for tracing to be enabled in child process. Fix that as well. BUG=709524,708487 R=primiano@chromium.org ========== to ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is also make startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=709524,708487 R=primiano@chromium.org ==========
Description was changed from ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is also make startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=709524,708487 R=primiano@chromium.org ========== to ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is makes startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=709524,708487 R=primiano@chromium.org ==========
Patchset #2 (id:20001) has been deleted
Yeah I didn't read the runloop part well. That wait was redundant. At least now we know thw real issue. fixing just periodic dumps. ptal thanks.
lgtm
Description was changed from ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is makes startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=709524,708487 R=primiano@chromium.org ========== to ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is makes startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=607533 R=primiano@chromium.org ==========
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492115937270650,
"parent_rev": "4f3c18cf2a88b9a88f42fc18461522f718685e59", "commit_rev":
"1a345de25847354fb1e1b1b81bbd00a64af8d55b"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is makes startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=607533 R=primiano@chromium.org ========== to ========== [memory-infra] Trigger first periodic dump after initial period The periodic dumps behavior was to create memory dump after the first period ended. It is makes startup tracing worse in case we want a single dump and not the dump at start. So, get back old behavior. BUG=607533 R=primiano@chromium.org Review-Url: https://codereview.chromium.org/2815963002 Cr-Commit-Position: refs/heads/master@{#464539} Committed: https://chromium.googlesource.com/chromium/src/+/1a345de25847354fb1e1b1b81bbd... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1a345de25847354fb1e1b1b81bbd... |
