|
|
Created:
4 years, 7 months ago by keishi Modified:
4 years, 7 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, darin-cc_chromium.org, jam, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd deep report for BENCHMARK_MEMORY
Adds BENCHMARK_MEMORY tracing config.
And adds deep report triggered on low memory.
BUG=612102
Committed: https://crrev.com/4743ab9d5ecd967717bcffe6e68063c0d81c1ff8
Cr-Commit-Position: refs/heads/master@{#396401}
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : limit to 5sec #
Total comments: 6
Patch Set 4 : fixed heavy #
Messages
Total messages: 27 (10 generated)
Description was changed from ========== Prepare for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. Whitelists memory-infra args for collection. BUG=None ========== to ========== Prepare for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. Whitelists memory-infra args for collection. BUG=612102 ==========
keishi@chromium.org changed reviewers: + oysteine@google.com
PTAL google3 side change: https://critique.corp.google.com/#review/122397505
primiano@chromium.org changed reviewers: + primiano@chromium.org
What does this cause if enabled? Is this for deep or bulk reports? As it is, memory-infra does not have any form of whitelisting. This should NOT land as it is if it's going to affect bulk reports.
On 2016/05/17 06:41:45, Primiano Tucci wrote: > What does this cause if enabled? Is this for deep or bulk reports? > As it is, memory-infra does not have any form of whitelisting. > This should NOT land as it is if it's going to affect bulk reports. The whitelist specifically affects bulk reports, as that's where we need to strip PII. If this category contains PII, then that's definitely a no-go. keishi: Maybe a version of this CL without the whitelisting added? If you want to experiment with the trace collection pipeline it may be better to start with deep reports while the PII situation is worked out.
Description was changed from ========== Prepare for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. Whitelists memory-infra args for collection. BUG=612102 ========== to ========== Add deep report for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. And adds deep report triggered on low memory. BUG=612102 ==========
I've modified the CL to use deep reports. I would like to start collecting/analyzing some data using deep reports while lightweight memory-infra with PII stripping is being implemented. +oystein I have a couple of questions. What does trigger_day property do? Unless I set it, BackgroundTracingManagerImpl::OnRuleTriggered calls StartTracing but then immediately calls BeginFinalizing because trace_delay is -1, resulting in a very short trace. How can I set excluded_categories? memory-infra is big so I was hoping to turn off other categories.
On 2016/05/24 06:40:09, keishi wrote: > I've modified the CL to use deep reports. I would like to start > collecting/analyzing some data using deep reports while lightweight memory-infra > with PII stripping is being implemented. > > +oystein I have a couple of questions. > > What does trigger_day property do? s/trigger_day/trigger_delay/
ssid@chromium.org changed reviewers: + ssid@chromium.org
Is there a plan to set different triggers for deep reports? Tracing for 30 seconds will generate huge trace files. I traced an empty page for 30 seconds and the trace viewer crashed while loading this file. So, I tried startup tracing for 30 secs: The browser memory peak increased by 80Mib : At some point in tracing it causes 80Mib increase in one process. The new tab page trace was 40Mib in size. Trace with few tabs open, it was 180Mib. I can imagine this scales to gigs per traces on desktop users. We don't want to be uploading such files even if it is desktop. Reducing the trigger_delay might not be the best option. There could be some cases when the dump takes seconds and making this delay to 2 secs might not produce a dump at all. I suggest either set a trace trigger with higher periodic interval (like 10secs) or just do one heavy dump per trigger since you want to know the memory use after a GC. Setting the time period can be done by: Changing the MemoryDumpConfig in the TraceConfig passed by background tracing controller only when BACKGROUND_MEMORY trigger is hit. See BackgroundTracingManagerImpl::StartTracingIfConfigNeedsIt Triggering a single dump is a little tricky since we need to trigger after all the processes have started tracing: BackgroundTracingManagerImpl::StartTracing passes a tracing_enabled_callback_for_testing_ this has to replaced with a callback for memory-infra which triggers a global dump once (MemoryDumpManager::RequestGlobalDump). https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... File content/browser/tracing/background_tracing_config_impl.h (right): https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config_impl.h:35: BENCHMARK_MEMORY, How do we differentiate between a deep report trigger and bulk report trigger? I was imagining BENCHMARK_MEMORY by default starts tracing using BACKGROUND mode in memory-infra (will be introduced). But if we need heavy modes in deep reports, we should name this BENCHMARK_MEMORY_HEAVY or something. https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_manager_impl.cc:522: return "disabled-by-default-memory-infra"; "-*,disabled-by-default-memory-infra" should exclude other categories. This will start tracing using the default config: every 250ms light dumps and 2s heavy dumps.
On 2016/05/25 02:02:13, ssid wrote: > Is there a plan to set different triggers for deep reports? What do you mean by trigger? This CL uses the V8.GCLowMemoryNotification histogram for the trigger because it already exists. But I plan to change to a better trigger after analyzing the data from this experiment. > Tracing for 30 seconds will generate huge trace files. You're saying 30seconds because of kStartTracingTimeoutSeconds? > Reducing the trigger_delay might not be the best option. There could be some > cases when the dump takes seconds and making this delay to 2 secs might not > produce a dump at all. I still have no idea what trigger_delay does. Does setting trigger_delay to 2 seconds mean tracing is 2 seconds? > Setting the time period can be done by: Changing the MemoryDumpConfig in the > TraceConfig passed by background tracing controller only when BACKGROUND_MEMORY > trigger is hit. See BackgroundTracingManagerImpl::StartTracingIfConfigNeedsIt I looked into this but I wasn't sure what the best way to do it was. Should I add a memory_dump_config dict to the BackgroundTracingConfig JSON? Or should I add a if statement that special cases for BACKGROUND_MEMORY.
oysteine@chromium.org changed reviewers: + oysteine@chromium.org
On 2016/05/25 13:27:23, keishi wrote: > On 2016/05/25 02:02:13, ssid wrote: > > Is there a plan to set different triggers for deep reports? > What do you mean by trigger? This CL uses the V8.GCLowMemoryNotification > histogram for the trigger because it already exists. But I plan to change to a > better trigger after analyzing the data from this experiment. > > > Tracing for 30 seconds will generate huge trace files. > You're saying 30seconds because of kStartTracingTimeoutSeconds? > > > Reducing the trigger_delay might not be the best option. There could be some > > cases when the dump takes seconds and making this delay to 2 secs might not > > produce a dump at all. > I still have no idea what trigger_delay does. Does setting trigger_delay to 2 > seconds mean tracing is 2 seconds? > Yep, the trigger_delay is the delay from when the trigger is hit, until tracing is stopped and finalized. > > Setting the time period can be done by: Changing the MemoryDumpConfig in the > > TraceConfig passed by background tracing controller only when > BACKGROUND_MEMORY > > trigger is hit. See BackgroundTracingManagerImpl::StartTracingIfConfigNeedsIt > I looked into this but I wasn't sure what the best way to do it was. > Should I add a memory_dump_config dict to the BackgroundTracingConfig JSON? > Or should I add a if statement that special cases for BACKGROUND_MEMORY. I think extending the BackgroundTracingConfig JSON is the right way to go. An end goal should be that we can embed a full TraceConfig into it (or merge the two even), but that may be out of scope here. https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... File content/browser/tracing/background_tracing_config_impl.h (right): https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... content/browser/tracing/background_tracing_config_impl.h:35: BENCHMARK_MEMORY, On 2016/05/25 02:02:13, ssid wrote: > How do we differentiate between a deep report trigger and bulk report trigger? I > was imagining BENCHMARK_MEMORY by default starts tracing using BACKGROUND mode > in memory-infra (will be introduced). But if we need heavy modes in deep > reports, we should name this BENCHMARK_MEMORY_HEAVY or something. These are just enums of hardcoded category lists, would the heavy and light memory-infra modes by configured through the set of enabled categories? Or something else?
On 2016/05/25 16:26:41, Oystein wrote: > On 2016/05/25 13:27:23, keishi wrote: > > On 2016/05/25 02:02:13, ssid wrote: > > > Is there a plan to set different triggers for deep reports? > > What do you mean by trigger? This CL uses the V8.GCLowMemoryNotification > > histogram for the trigger because it already exists. But I plan to change to a > > better trigger after analyzing the data from this experiment. > > Sorry I meant MemoryDumpTriggers for trace config. To set the interval and dump mode. > > > > Setting the time period can be done by: Changing the MemoryDumpConfig in the > > > TraceConfig passed by background tracing controller only when > > BACKGROUND_MEMORY > > > trigger is hit. See > BackgroundTracingManagerImpl::StartTracingIfConfigNeedsIt > > I looked into this but I wasn't sure what the best way to do it was. > > Should I add a memory_dump_config dict to the BackgroundTracingConfig JSON? > > Or should I add a if statement that special cases for BACKGROUND_MEMORY. > > > I think extending the BackgroundTracingConfig JSON is the right way to go. An > end goal should be that we can embed a full TraceConfig into it (or merge the > two even), but that may be out of scope here. Yes that is the issue I am talking about here. We do not have support for passing trace config strings from finch. So, the only way to differentiate between heavy and light trigger should be part of the enum BENCHMARK_MEMORY_HEAVY. > https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... > File content/browser/tracing/background_tracing_config_impl.h (right): > > https://codereview.chromium.org/1981823003/diff/20001/content/browser/tracing... > content/browser/tracing/background_tracing_config_impl.h:35: BENCHMARK_MEMORY, > On 2016/05/25 02:02:13, ssid wrote: > > How do we differentiate between a deep report trigger and bulk report trigger? > I > > was imagining BENCHMARK_MEMORY by default starts tracing using BACKGROUND mode > > in memory-infra (will be introduced). But if we need heavy modes in deep > > reports, we should name this BENCHMARK_MEMORY_HEAVY or something. > > These are just enums of hardcoded category lists, would the heavy and light > memory-infra modes by configured through the set of enabled categories? Or > something else? They are configured using the trace config strings. The default config is taken if tracing is started using trace filter strings. The default triggers are heavy for background tracing even for deep reports as i mentioned.
For this first experiment I would like to collect: - at least two detailed dumps to observe the memory usage distribution in a low memory situation - around 5 seconds worth of light dumps to observe how the memory usage transitions after a low memory warning. If I set the tracing delay to 5 seconds I got around 2-3 detailed dumps and the trace file size was 5MB for 25 tabs. So I'm thinking the default memory dump intervals should be fine. Still renamed the category to BENCHMARK_MEMORY_HEAVY because we may want to tweak the memory dump config for a later experiment.
On 2016/05/26 05:37:04, keishi wrote: > For this first experiment I would like to collect: > > - at least two detailed dumps to observe the memory usage distribution in a low > memory situation > > - around 5 seconds worth of light dumps to observe how the memory usage > transitions after a low memory warning. > > If I set the tracing delay to 5 seconds I got around 2-3 detailed dumps and the > trace file size was 5MB for 25 tabs. So I'm thinking the default memory dump > intervals should be fine. The problem is you might not get a dump at all if there were other tasks running in some task runners and dump tasks were delayed in those 5 secs or one of the processes is slow. But it wouldn't happen often, so this should be fine. > Still renamed the category to BENCHMARK_MEMORY_HEAVY because we may want to > tweak the memory dump config for a later experiment.
lgtm, let's see what we get!
Looks good. Please rename to heavy everywhere. https://codereview.chromium.org/1981823003/diff/40001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.cc (right): https://codereview.chromium.org/1981823003/diff/40001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.cc:78: rules_dict->SetString("category", "BENCHMARK_MEMORY"); HEAVY https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_config_impl.cc (right): https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_config_impl.cc:34: const char kConfigCategoryBenchmarkMemory[] = "BENCHMARK_MEMORY_HEAVY"; kConfigCategoryBenchmarkMemoryHeavy https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_config_unittest.cc (right): https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_config_unittest.cc:297: "BENCHMARK_MEMORY", ditto
Thanks! https://codereview.chromium.org/1981823003/diff/40001/chrome/browser/tracing/... File chrome/browser/tracing/navigation_tracing.cc (right): https://codereview.chromium.org/1981823003/diff/40001/chrome/browser/tracing/... chrome/browser/tracing/navigation_tracing.cc:78: rules_dict->SetString("category", "BENCHMARK_MEMORY"); On 2016/05/27 01:08:10, ssid wrote: > HEAVY Done. https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_config_impl.cc (right): https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_config_impl.cc:34: const char kConfigCategoryBenchmarkMemory[] = "BENCHMARK_MEMORY_HEAVY"; On 2016/05/27 01:08:10, ssid wrote: > kConfigCategoryBenchmarkMemoryHeavy Done. https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... File content/browser/tracing/background_tracing_config_unittest.cc (right): https://codereview.chromium.org/1981823003/diff/40001/content/browser/tracing... content/browser/tracing/background_tracing_config_unittest.cc:297: "BENCHMARK_MEMORY", On 2016/05/27 01:08:10, ssid wrote: > ditto Done.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/1981823003/#ps60001 (title: "fixed heavy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981823003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981823003/60001
Message was sent while issue was closed.
Description was changed from ========== Add deep report for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. And adds deep report triggered on low memory. BUG=612102 ========== to ========== Add deep report for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. And adds deep report triggered on low memory. BUG=612102 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add deep report for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. And adds deep report triggered on low memory. BUG=612102 ========== to ========== Add deep report for BENCHMARK_MEMORY Adds BENCHMARK_MEMORY tracing config. And adds deep report triggered on low memory. BUG=612102 Committed: https://crrev.com/4743ab9d5ecd967717bcffe6e68063c0d81c1ff8 Cr-Commit-Position: refs/heads/master@{#396401} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4743ab9d5ecd967717bcffe6e68063c0d81c1ff8 Cr-Commit-Position: refs/heads/master@{#396401} |