Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912483002:
Change memory dumps to not use periodic dumps by default. (Closed)
|
Created:
3 years, 7 months ago by erikchen Modified:
3 years, 6 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, ssid Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange memory dumps to not use periodic dumps by default.
The only consumer that requires periodic dumps [and doesn't specify an explicit
dump config] is Chrome tracing, so explicitly specify a periodic dump config
there. Remove by default the periodic dump config.
BUG=726393
Review-Url: https://codereview.chromium.org/2912483002
Cr-Commit-Position: refs/heads/master@{#475608}
Committed: https://chromium.googlesource.com/chromium/src/+/0529413d84c06f3720bf4a56b28bb2c7358cefd1
Patch Set 1 #
Total comments: 6
Patch Set 2 : comments from primiano. #
Total comments: 2
Patch Set 3 : Comments from ssid. #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by erikchen@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...
erikchen@chromium.org changed reviewers: + primiano@chromium.org
primiano: Please review. Note that this temporarily breaks trace-on-tap, which relies on the existence of default dumps, but that's an experimental extension which requires this change in order to be fixed to work better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
primiano@chromium.org changed reviewers: + ssid@chromium.org
+ssid can you give another pair of eyes? I quickly glanced to this from a phone and seems okay % small comments. Rs-lgtm after ssid makes a pass https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:95: base::trace_event::MemoryDumpManager::kTraceCategory) != Iirc there is a iscategoryenabled in traceconfig that makes this a bit cleaner. The real bug here is that I think somebody created a memory-infra.V8 which adds some extra expensive stuff. This string matching will get also that. Is category enabled() will not. https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:104: base::trace_event::MemoryDumpLevelOfDetail::DETAILED); Check with ssid, I don't think you need this. This should be only for restricting, this one here should be the default behavior.
https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:95: base::trace_event::MemoryDumpManager::kTraceCategory) != On 2017/05/26 19:12:33, Primiano Tucci wrote: > Iirc there is a iscategoryenabled in traceconfig that makes this a bit cleaner. > The real bug here is that I think somebody created a memory-infra.V8 which adds > some extra expensive stuff. This string matching will get also that. Is category > enabled() will not. Done. https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:104: base::trace_event::MemoryDumpLevelOfDetail::DETAILED); On 2017/05/26 19:12:33, Primiano Tucci wrote: > Check with ssid, I don't think you need this. This should be only for > restricting, this one here should be the default behavior. This is necessary, because a freshly constructed MemoryDumpConfig doesn't pick up the default behavior. *shrug*. You're welcome to fix this. :)
The CQ bit was checked by erikchen@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...
lgtm % the category check https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:104: base::trace_event::MemoryDumpLevelOfDetail::DETAILED); On 2017/05/26 20:31:13, erikchen wrote: > On 2017/05/26 19:12:33, Primiano Tucci wrote: > > Check with ssid, I don't think you need this. This should be only for > > restricting, this one here should be the default behavior. > > This is necessary, because a freshly constructed MemoryDumpConfig doesn't pick > up the default behavior. *shrug*. You're welcome to fix this. :) Yeah we only thought about the trace config string case where if it is not specified then all modes are assumed. While constructing the object itself, we need to add modes manually. meh this is unfortunate. add a todo for me. https://codereview.chromium.org/2912483002/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/20001/content/browser/tracing... content/browser/tracing/tracing_ui.cc:94: if (trace_config->category_filter().IsCategoryEnabled( This should just be trace/config->IsCategoryGroupEnabled In future if we change memory-infra to be default category (no disabled-by-default) then this function will still return false but the other will return true. In general this api (category_filter()) should not be exposed. This should be fixed, seems to be used only in tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/1/content/browser/tracing/tra... content/browser/tracing/tracing_ui.cc:104: base::trace_event::MemoryDumpLevelOfDetail::DETAILED); On 2017/05/26 21:56:07, ssid wrote: > On 2017/05/26 20:31:13, erikchen wrote: > > On 2017/05/26 19:12:33, Primiano Tucci wrote: > > > Check with ssid, I don't think you need this. This should be only for > > > restricting, this one here should be the default behavior. > > > > This is necessary, because a freshly constructed MemoryDumpConfig doesn't pick > > up the default behavior. *shrug*. You're welcome to fix this. :) > > Yeah we only thought about the trace config string case where if it is not > specified then all modes are assumed. While constructing the object itself, we > need to add modes manually. meh this is unfortunate. add a todo for me. Done. https://codereview.chromium.org/2912483002/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_ui.cc (right): https://codereview.chromium.org/2912483002/diff/20001/content/browser/tracing... content/browser/tracing/tracing_ui.cc:94: if (trace_config->category_filter().IsCategoryEnabled( On 2017/05/26 21:56:07, ssid wrote: > This should just be trace/config->IsCategoryGroupEnabled > In future if we change memory-infra to be default category (no > disabled-by-default) then this function will still return false but the other > will return true. In general this api (category_filter()) should not be exposed. > This should be fixed, seems to be used only in tests. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2912483002/#ps40001 (title: "Comments from ssid.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by erikchen@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": 1496163501083170,
"parent_rev": "f0c5f7411ded1817dc5e1a293f87c7c50c5fb6c4", "commit_rev":
"0529413d84c06f3720bf4a56b28bb2c7358cefd1"}
Message was sent while issue was closed.
Description was changed from ========== Change memory dumps to not use periodic dumps by default. The only consumer that requires periodic dumps [and doesn't specify an explicit dump config] is Chrome tracing, so explicitly specify a periodic dump config there. Remove by default the periodic dump config. BUG=726393 ========== to ========== Change memory dumps to not use periodic dumps by default. The only consumer that requires periodic dumps [and doesn't specify an explicit dump config] is Chrome tracing, so explicitly specify a periodic dump config there. Remove by default the periodic dump config. BUG=726393 Review-Url: https://codereview.chromium.org/2912483002 Cr-Commit-Position: refs/heads/master@{#475608} Committed: https://chromium.googlesource.com/chromium/src/+/0529413d84c06f3720bf4a56b28b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0529413d84c06f3720bf4a56b28b...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2952693002/ by erikchen@chromium.org. The reason for reverting is: This breaks periodic dumping on Android. Reverting.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
