Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899683002:
Reenable Purge+Suspend Memory Growth metrics. (Closed)
DescriptionReenable Purge+Suspend Memory Growth metrics.
Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different
scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not.
BUG=722792, 670539, 635419
Review-Url: https://codereview.chromium.org/2899683002
Cr-Commit-Position: refs/heads/master@{#474197}
Committed: https://chromium.googlesource.com/chromium/src/+/a27961af152737aeb934c77180b9667e51bbee06
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed. #
Total comments: 4
Patch Set 3 : Fixed. #Messages
Total messages: 31 (22 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...
Description was changed from ========== Enable Purge+Suspend Memory Growth metrics. Since UMA_HISTOGRAM macro defines a static variable, need to use different scope for each histogram name. BUG=722792, 670539, 635419 ========== to ========== Enable Purge+Suspend Memory Growth metrics. Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not. BUG=722792, 670539, 635419 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tasak@google.com changed reviewers: + avi@chromium.org, isherman@chromium.org
Would you review this CL? This patch is for fixing the following DCHECK failure: [FATAL:histogram_base.cc(73)] Check failed: histogram_name() == name (PurgeAndSuspend.Experimental.MemoryGrowth.PartitionAllocKB. 30min vs. PurgeAndSuspend.Experimental.MemoryGrowth.PartitionAllocKB.60min) #0 0x7f520252485b base::debug::StackTrace::StackTrace() #1 0x7f520252355c base::debug::StackTrace::StackTrace() #2 0x7f5202597113 logging::LogMessage::~LogMessage() #3 0x7f52025fb080 base::HistogramBase::CheckName() #4 0x7f51fcab6137 content::RenderThreadImpl::RecordPurgeAndSuspendMemoryGrowthMetrics()
Description was changed from ========== Enable Purge+Suspend Memory Growth metrics. Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not. BUG=722792, 670539, 635419 ========== to ========== Reenable Purge+Suspend Memory Growth metrics. Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not. BUG=722792, 670539, 635419 ==========
lgtm stamp
Sorry for not catching the misuse of the histogram macros during my previous code review -- I somehow convinced myself that the names were runtime constants per-callsite. https://codereview.chromium.org/2899683002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2899683002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1841: UMA_HISTOGRAM_MEMORY_KB( \ Could you use base::UmaHistogramMemoryKB() and avoid the messy macros? https://codereview.chromium.org/2899683002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1871: NOTREACHED(); nit: In general, it's discouraged to have default cases in switch statements. Without a default stmt, the compiler can catch omitted cases and issue a warning.
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/2899683002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2899683002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1841: UMA_HISTOGRAM_MEMORY_KB( \ On 2017/05/22 20:28:01, Ilya Sherman wrote: > Could you use base::UmaHistogramMemoryKB() and avoid the messy macros? Done. https://codereview.chromium.org/2899683002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1871: NOTREACHED(); On 2017/05/22 20:28:01, Ilya Sherman wrote: > nit: In general, it's discouraged to have default cases in switch statements. > Without a default stmt, the compiler can catch omitted cases and issue a > warning. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1818: #define UMA_PURGE_AND_SUSPEND_MEMORY_GROWTH(suffix, memory_metrics) \ Rather than using a macro here, please revert to passing a string suffix, and just append the suffix below. https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1820: "PurgeAndSuspend.Experimental.MemoryGrowth.PartitionAllocKB" suffix, \ By the way, I think you lost the final dot in the histogram name?
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...
https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1818: #define UMA_PURGE_AND_SUSPEND_MEMORY_GROWTH(suffix, memory_metrics) \ On 2017/05/23 15:21:58, Ilya Sherman wrote: > Rather than using a macro here, please revert to passing a string suffix, and > just append the suffix below. I see. I misunderstood the previous comment. So by using base::UmaHistogramMemoryKB, it is possible to use generated histogram names.... Done. https://codereview.chromium.org/2899683002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:1820: "PurgeAndSuspend.Experimental.MemoryGrowth.PartitionAllocKB" suffix, \ On 2017/05/23 15:21:58, Ilya Sherman wrote: > By the way, I think you lost the final dot in the histogram name? Yeah, it's a critical bug... Thanks. Fixed by reverting the code.
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...
LGTM, thanks.
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 tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2899683002/#ps60001 (title: "Fixed.")
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": 60001, "attempt_start_ts": 1495610933283310,
"parent_rev": "491c6b2fc5e9d5fd2b7eada34026ff977797b493", "commit_rev":
"a27961af152737aeb934c77180b9667e51bbee06"}
Message was sent while issue was closed.
Description was changed from ========== Reenable Purge+Suspend Memory Growth metrics. Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not. BUG=722792, 670539, 635419 ========== to ========== Reenable Purge+Suspend Memory Growth metrics. Fix DCHECK failure in base::HistogramBase::CheckName(). We need to use different scope for each histogram name, because UMA_HISTOGRAM macro defines a static variable and checks whether the histogram name stored in the variable is the same as newly given histogram names or not. BUG=722792, 670539, 635419 Review-Url: https://codereview.chromium.org/2899683002 Cr-Commit-Position: refs/heads/master@{#474197} Committed: https://chromium.googlesource.com/chromium/src/+/a27961af152737aeb934c77180b9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a27961af152737aeb934c77180b9... |
