Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843373004:
Record renderer's memory growth 30min, 60min and 90min after purging separately. (Closed)
DescriptionRecord renderer's memory growth 30min, 60min and 90min after purging separately.
We would like to compare 30 min's metrics with 60 and 90 min's data.
BUG=670539, 635419
Review-Url: https://codereview.chromium.org/2843373004
Cr-Commit-Position: refs/heads/master@{#471706}
Committed: https://chromium.googlesource.com/chromium/src/+/bb0640b700217f492e6d68bd2f3a31afea8c8c98
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated #
Total comments: 2
Patch Set 3 : Updated #
Total comments: 2
Patch Set 4 : Fixed comment #
Messages
Total messages: 43 (30 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...
Patchset #1 (id:1) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add PurgeAndSuspend.Experimental.MemoryGrowth.*.30min,60min,90min BUG= ========== to ========== Record renderer's memory growth 30min, 60min and 90min after purging separately. We would like to compare 30 min's metrics with 60 and 90 min's data. BUG= ==========
Patchset #1 (id:20001) has been deleted
Patchset #1 (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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tasak@google.com changed reviewers: + isherman@chromium.org
Would you review this CL?
I'm not sure whether this is correct way to add metrics under
PurgeAndSuspend.Experimental.MemoryGrowth.{PartitionAlloc}...}.
So I would like to get your advice.
Note that you're tripling the storage requirements -- both in memory on the client and in all forms on the server -- by splitting apart these metrics. Please consider carefully whether you really need the extra detail, and whether you need it indefinitely or only for a brief duration. https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1694: base::TimeDelta::FromMinutes(15)); I think this previous structure was both simpler and clearer. Why did you decide to move away from it? IMO it would be much nicer to simply bind the strings "30min" etc. here. https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1845: UMA_HISTOGRAM_MEMORY_GROWTH(".90min"); Optional nit: I think it'd be a bit cleaner to have the dot included in the macro. https://codereview.chromium.org/2843373004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2843373004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90213: - <suffix name="TotalAllocatedKB" Please mark these as <obsolete> rather than removing them. Actually, even better would be to mark them as base="true", and use an additional histogram_suffixes element to further complete the names to .30min, etc.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thank you for review. On 2017/05/09 03:58:58, Ilya Sherman wrote: > Note that you're tripling the storage requirements -- both in memory on the > client and in all forms on the server -- by splitting apart these metrics. > Please consider carefully whether you really need the extra detail, and whether > you need it indefinitely or only for a brief duration. Yeah... So we are now planning to update purge+throttle. I'm not sure when we should run "purge" again. I would like to investigate what happens X minutes after purging from the viewpoint of memory... (currently X is 30, 60 and 90). https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1694: base::TimeDelta::FromMinutes(15)); On 2017/05/09 03:58:58, Ilya Sherman wrote: > I think this previous structure was both simpler and clearer. Why did you > decide to move away from it? IMO it would be much nicer to simply bind the > strings "30min" etc. here. Done. https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2843373004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:1845: UMA_HISTOGRAM_MEMORY_GROWTH(".90min"); On 2017/05/09 03:58:58, Ilya Sherman wrote: > Optional nit: I think it'd be a bit cleaner to have the dot included in the > macro. Done. https://codereview.chromium.org/2843373004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2843373004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90213: - <suffix name="TotalAllocatedKB" On 2017/05/09 03:58:58, Ilya Sherman wrote: > Please mark these as <obsolete> rather than removing them. Actually, even > better would be to mark them as base="true", and use an additional > histogram_suffixes element to further complete the names to .30min, etc. Done.
Thanks. I do not think I'm sufficiently familiar with the RenderThreadImpl code to be a great choice of primary reviewer for that code. Please add an additional reviewer who's more familiar with that code. https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... content/renderer/render_thread_impl.cc:1683: base::Unretained(this), "30min", process_foregrounded_count_), Are you sure that base::Unretained is safe? What makes it safe? https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... content/renderer/render_thread_impl.h:788: unsigned process_foregrounded_count_; I don't think that "unsigned" is the most appropriate type for this variable. I'd probably recommend int. PTAL at https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... for more info. https://codereview.chromium.org/2843373004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843373004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:90323: + purging"/> Hmm, why did you choose not to use an additional layer of histogram_suffixes to expand out these names?
Patchset #2 (id:80001) 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. I'm sorry. I took a mistake... I deleted the patchset 1. On 2017/05/11 00:14:06, Ilya Sherman wrote: > Thanks. I do not think I'm sufficiently familiar with the RenderThreadImpl code > to be a great choice of primary reviewer for that code. Please add an > additional reviewer who's more familiar with that code. > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > content/renderer/render_thread_impl.cc:1683: base::Unretained(this), "30min", > process_foregrounded_count_), > Are you sure that base::Unretained is safe? What makes it safe? Yes, it's safe. Because RenderThreadImpl is kept alive as its render process is alive. I looked at RenderMain(). It invokes RenderThreadImpl::Create(), but it doesn't keep the Create()'s return value. Instead, thread local value holds the created RenderThreadImpl (c.f. RenderThreadImpl::current()). > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > File content/renderer/render_thread_impl.h (right): > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > content/renderer/render_thread_impl.h:788: unsigned process_foregrounded_count_; > I don't think that "unsigned" is the most appropriate type for this variable. > I'd probably recommend int. PTAL at > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > for more info. I see. Done. > https://codereview.chromium.org/2843373004/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2843373004/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:90323: + purging"/> > Hmm, why did you choose not to use an additional layer of histogram_suffixes to > expand out these names? I see. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. LGTM % remaining comment on histogram_suffixes. Please do add an additional reviewer for the render_thread_Impl changes. On 2017/05/11 09:17:02, tasak wrote: > On 2017/05/11 00:14:06, Ilya Sherman wrote: > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > > File content/renderer/render_thread_impl.cc (right): > > > > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > > content/renderer/render_thread_impl.cc:1683: base::Unretained(this), "30min", > > process_foregrounded_count_), > > Are you sure that base::Unretained is safe? What makes it safe? > > Yes, it's safe. Because RenderThreadImpl is kept alive as its render process is > alive. > I looked at RenderMain(). It invokes RenderThreadImpl::Create(), but it doesn't > keep the Create()'s return value. > Instead, thread local value holds the created RenderThreadImpl (c.f. > RenderThreadImpl::current()). If I'm understanding correctly, you're saying that this object is never freed/deleted, but rather simply cleaned up as part of process teardown? If so, then I agree, it should be safe to use base::Unretained. It might be worth documenting this for the next reader. Thanks! https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:90298: + </histogram_suffixes> Hmm, I don't think it's valid to nest histogram_suffixes elements. (Or did you try running a script to generate full histogram names, and verify that this works? Perhaps the syntax has been updated to be more flexible than I was aware of.) I think you need to move this to follow the existing histogram_suffixes element, and give it a different name, like "PurgeAndSuspendExperimentPerHalfHour". (Though, I'm not actually sure where, if anywhere, we use the name attributes for histogram_suffixes elements. It might just be for alphabetizing within this file...)
Thank you for review. On 2017/05/11 20:34:32, Ilya Sherman wrote: > Thanks. LGTM % remaining comment on histogram_suffixes. Please do add an > additional reviewer for the render_thread_Impl changes. > > On 2017/05/11 09:17:02, tasak wrote: > > On 2017/05/11 00:14:06, Ilya Sherman wrote: > > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > > > File content/renderer/render_thread_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2843373004/diff/80001/content/renderer/render... > > > content/renderer/render_thread_impl.cc:1683: base::Unretained(this), > "30min", > > > process_foregrounded_count_), > > > Are you sure that base::Unretained is safe? What makes it safe? > > > > Yes, it's safe. Because RenderThreadImpl is kept alive as its render process > is > > alive. > > I looked at RenderMain(). It invokes RenderThreadImpl::Create(), but it > doesn't > > keep the Create()'s return value. > > Instead, thread local value holds the created RenderThreadImpl (c.f. > > RenderThreadImpl::current()). > > If I'm understanding correctly, you're saying that this object is never > freed/deleted, but rather simply cleaned up as part of process teardown? If so, > then I agree, it should be safe to use base::Unretained. It might be worth > documenting this for the next reader. Thanks! Yeah, never deleted. Instead, just a render process uses exit() to delete RenderThreadImpl. I added a comment about RenderThreadImpl's lifetime. > https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:90298: + </histogram_suffixes> > Hmm, I don't think it's valid to nest histogram_suffixes elements. (Or did you > try running a script to generate full histogram names, and verify that this > works? Perhaps the syntax has been updated to be more flexible than I was aware > of.) > > I think you need to move this to follow the existing histogram_suffixes element, > and give it a different name, like "PurgeAndSuspendExperimentPerHalfHour". > (Though, I'm not actually sure where, if anywhere, we use the name attributes > for histogram_suffixes elements. It might just be for alphabetizing within this > file...)
https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2843373004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:90298: + </histogram_suffixes> On 2017/05/11 20:34:32, Ilya Sherman wrote: > Hmm, I don't think it's valid to nest histogram_suffixes elements. (Or did you > try running a script to generate full histogram names, and verify that this > works? Perhaps the syntax has been updated to be more flexible than I was aware > of.) > > I think you need to move this to follow the existing histogram_suffixes element, > and give it a different name, like "PurgeAndSuspendExperimentPerHalfHour". > (Though, I'm not actually sure where, if anywhere, we use the name attributes > for histogram_suffixes elements. It might just be for alphabetizing within this > file...) I see. Done.
tasak@google.com changed reviewers: + avi@chromium.org
avi, would you review this CL?
Code LGTM but you need a bug in the description to attach this to. https://codereview.chromium.org/2843373004/diff/120001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2843373004/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:1681: // so it is possible to use base::Unretained(this) here. grammar nit: drop "so". "Since RenderThreadImpl is kept alive [...] it is possible to use base::Unretained [...]"
Description was changed from ========== Record renderer's memory growth 30min, 60min and 90min after purging separately. We would like to compare 30 min's metrics with 60 and 90 min's data. BUG= ========== to ========== Record renderer's memory growth 30min, 60min and 90min after purging separately. We would like to compare 30 min's metrics with 60 and 90 min's data. BUG=670539,635419 ==========
Thank you for review. I added purge+throttle bug id. https://codereview.chromium.org/2843373004/diff/120001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2843373004/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:1681: // so it is possible to use base::Unretained(this) here. On 2017/05/12 15:41:23, Avi (ping after 24h) wrote: > grammar nit: drop "so". > > "Since RenderThreadImpl is kept alive [...] it is possible to use > base::Unretained [...]" Done.
The CQ bit was checked by tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2843373004/#ps140001 (title: "Fixed comment")
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": 140001, "attempt_start_ts": 1494831037813390,
"parent_rev": "6487fce3748547e7a27b52e11be877413d05199a", "commit_rev":
"bb0640b700217f492e6d68bd2f3a31afea8c8c98"}
Message was sent while issue was closed.
Description was changed from ========== Record renderer's memory growth 30min, 60min and 90min after purging separately. We would like to compare 30 min's metrics with 60 and 90 min's data. BUG=670539,635419 ========== to ========== Record renderer's memory growth 30min, 60min and 90min after purging separately. We would like to compare 30 min's metrics with 60 and 90 min's data. BUG=670539,635419 Review-Url: https://codereview.chromium.org/2843373004 Cr-Commit-Position: refs/heads/master@{#471706} Committed: https://chromium.googlesource.com/chromium/src/+/bb0640b700217f492e6d68bd2f3a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bb0640b700217f492e6d68bd2f3a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
