Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636873002:
memory metrics: record growth after purging BG renderer's cache (Closed)
Descriptionmemory metrics: record growth after purging BG renderer's cache
Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged.
BUG=635419
Review-Url: https://codereview.chromium.org/2636873002
Cr-Commit-Position: refs/heads/master@{#444722}
Committed: https://chromium.googlesource.com/chromium/src/+/72f64044b4e3a3826cc449c19c9e5947f931c667
Patch Set 1 #Patch Set 2 : Update histograms.xml #
Total comments: 4
Patch Set 3 : Use histogram_suffixes #
Total comments: 6
Patch Set 4 : Added label to PurgeAndSuspendExperiment. #
Messages
Total messages: 36 (24 generated)
The CQ bit was checked by tasak@google.com 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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Record memory growth after purging background renderer's cache. BUG= ========== to ========== Record memory growth after purging background renderer's cache. 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged, record memory growth of each allocator / total memory growth. BUG= ==========
Description was changed from ========== Record memory growth after purging background renderer's cache. 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged, record memory growth of each allocator / total memory growth. BUG= ========== to ========== Record memory growth after purging background renderer's cache. Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged. BUG=635419 ==========
tasak@google.com changed reviewers: + haraken@chromium.org, isherman@chromium.org, primiano@chromium.org
Would you review this CL?
From Memory Coordinator perspective, this looks good but primiano@ should confirm.
https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51747: + purged. Please document when this is recorded -- i.e. 5, 10, and 15 minutes after the purge (right?) https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51793: +</histogram> Please use histogram_suffixes to reduce the amount of repetition in this file.
The CQ bit was checked by tasak@google.com 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...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by tasak@google.com 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...
Thank you for review. https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51747: + purged. On 2017/01/19 02:26:09, Ilya Sherman wrote: > Please document when this is recorded -- i.e. 5, 10, and 15 minutes after the > purge (right?) Right. Done. https://codereview.chromium.org/2636873002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51793: +</histogram> On 2017/01/19 02:26:09, Ilya Sherman wrote: > Please use histogram_suffixes to reduce the amount of repetition in this file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics LGTM % a nit: https://codereview.chromium.org/2636873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115929: + <suffix name="BlinkGCKB"/> nit: Please add a brief label for each suffix.
LGTM primiano@ ?
LGTM with just some questions P.S. I'd reword a little bit the CL title to make it evident this is about measuring memory and does not affect the actual behavior of P&S. It usually help to rule out CLs during perf sheriff shifts. Something like: memory metrics: record growth after purging BG renderer's cache .... https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1796: base::TimeDelta::FromMinutes(5)); just checking: aren't timer throttled in bg? if so won't these timers be potentially not run? https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1800: GetRendererScheduler()->DefaultTaskRunner()->PostDelayedTask( Is GetRendererScheduler()->DefaultTaskRunner() the same task runner on which this method (RecordPurgeAndSuspendMetrics) runs? I'm asking because you are accessing purge_and_suspend_memory_metrics_ from both, so if they happen on two different threads there would be a race.
The CQ bit was checked by tasak@google.com 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...
Description was changed from ========== Record memory growth after purging background renderer's cache. Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged. BUG=635419 ========== to ========== memory metrics: record growth after purging BG renderer's cache Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged. BUG=635419 ==========
Thank you for review. https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1796: base::TimeDelta::FromMinutes(5)); On 2017/01/19 09:56:10, Primiano Tucci wrote: > just checking: aren't timer throttled in bg? if so won't these timers be > potentially not run? I think, default task runner is not throttled in bg. As far as I tested (by using fprintf), the record_purge_suspend_growth_metric_closure is invoked correctly - i.e. 5min, 10min and 15min after each purge. https://codereview.chromium.org/2636873002/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1800: GetRendererScheduler()->DefaultTaskRunner()->PostDelayedTask( On 2017/01/19 09:56:10, Primiano Tucci wrote: > Is GetRendererScheduler()->DefaultTaskRunner() the same task runner on which > this method (RecordPurgeAndSuspendMetrics) runs? I'm asking because you are > accessing purge_and_suspend_memory_metrics_ from both, so if they happen on two > different threads there would be a race. Yes, the same task runner. i.e. GetRendererScheduler()->DefaultTaskRunner()->PostDelayedTask( FROM_HERE, record_purge_suspend_metric_closure_.callback(), base::TimeDelta::FromSeconds(15)); https://codereview.chromium.org/2636873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2636873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115929: + <suffix name="BlinkGCKB"/> On 2017/01/19 08:07:21, Ilya Sherman wrote: > nit: Please add a brief label for each suffix. Done.
tasak@google.com changed reviewers: + kinuko@chromium.org
kinuko-san, rstamp please. I need lgtm from content owner. avi is not around. Thanks.
lgtm
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 haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2636873002/#ps80001 (title: "Added label to PurgeAndSuspendExperiment.")
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": 80001, "attempt_start_ts": 1484834280237550,
"parent_rev": "76a6b9b24f09b5d684f5305c1d60e6b8a2b745d2", "commit_rev":
"72f64044b4e3a3826cc449c19c9e5947f931c667"}
Message was sent while issue was closed.
Description was changed from ========== memory metrics: record growth after purging BG renderer's cache Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged. BUG=635419 ========== to ========== memory metrics: record growth after purging BG renderer's cache Record memory growth of each allocator / total memory growth 5 minutes, 10 minutes and 15 minutes after a backgrounded renderer is purged. BUG=635419 Review-Url: https://codereview.chromium.org/2636873002 Cr-Commit-Position: refs/heads/master@{#444722} Committed: https://chromium.googlesource.com/chromium/src/+/72f64044b4e3a3826cc449c19c9e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/72f64044b4e3a3826cc449c19c9e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
