Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925073002:
Move malloc/partition_alloc memory usage functions to base (Closed)
|
Created:
3 years, 6 months ago by keishi Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, blink-reviews, danakj+watch_chromium.org, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove malloc memory usage function to base
Move malloc memory usage function to base so they can be used inside Blink as well as in content/
BUG=None
Review-Url: https://codereview.chromium.org/2925073002
Cr-Commit-Position: refs/heads/master@{#480784}
Committed: https://chromium.googlesource.com/chromium/src/+/12b598b94796f25a6207e89eb844d2c73145858e
Patch Set 1 #Patch Set 2 : Moved to process_metrics #
Total comments: 6
Patch Set 3 : Moved to ProcessMetrics method, etc #
Messages
Total messages: 35 (15 generated)
keishi@chromium.org changed reviewers: + tasak@google.com
PTAL
Description was changed from ========== Move malloc/partition_alloc memory usage functions to base BUG= ========== to ========== Move malloc/partition_alloc memory usage functions to base Move malloc/partition_alloc memory usage functions to base so they can be used inside Blink as well as in content/ BUG= ==========
lgtm
Description was changed from ========== Move malloc/partition_alloc memory usage functions to base Move malloc/partition_alloc memory usage functions to base so they can be used inside Blink as well as in content/ BUG= ========== to ========== Move malloc/partition_alloc memory usage functions to base Move malloc/partition_alloc memory usage functions to base so they can be used inside Blink as well as in content/ BUG=None ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, primiano@chromium.org
PTAL I would like move these so I can send memory usage UMAs from inside Blink.
On 2017/06/08 11:51:02, keishi wrote: > PTAL I would like move these so I can send memory usage UMAs from inside Blink. Can I ask what is the use of this? We are getting soon to the state where you can get this data from the memory-infra service. What is the place in content where we'd use this? The only thing that feels odd to me is that we'd end up with two places in base (this one and base/trace_event/malloc_dump_provider.cc) that do 90% (but not 100%) the same stuff. If there is any urgency about getting the malloc data I'd at least factor out the common bits and have _one_ function in base that is used by both malloc_dump_provider and content, not duplicating the two.
On 2017/06/08 13:27:59, Primiano Tucci wrote: > On 2017/06/08 11:51:02, keishi wrote: > > PTAL I would like move these so I can send memory usage UMAs from inside > Blink. > > Can I ask what is the use of this? We are getting soon to the state where you > can get this data from the memory-infra service. What is the place in content > where we'd use this? > The only thing that feels odd to me is that we'd end up with two places in base > (this one and base/trace_event/malloc_dump_provider.cc) that do 90% (but not > 100%) the same stuff. > If there is any urgency about getting the malloc data I'd at least factor out > the common bits and have _one_ function in base that is used by both > malloc_dump_provider and content, not duplicating the two. We already have this function in render_thread_impl.cc. It was used to send UMAs (I understand that its because memory-infra wasn't ready). I am planning to land a memory reduction change to M61 and I would like to measure the reduction directly. Since the change is in Blink I would like to access these functions from Blink. If the new and proper way to get memory usage data from memory-infra is ready for M61, I am happy to use it.
From the technical perspective, In the best case this seems to belong to base/process_metrics.cc, which already does lot of computation for memory stuff and at that point would be great to refactor malloc_dump_provider to use that. Taking a step back and going to the general problem, I wonder if we should have some more general discussion on what is the right metric for whatever the change will be in M61. It can be quite easy to accidentally come up with a metric that represents something that won't make a difference for end users. Concrete exmaples: what if you measure memory before and after, but then few seconds later, that memory gets fully reacquired (or worse, there is some bug and some code ends up acquiring 2x the original memory?)
On 2017/06/08 14:41:21, Primiano Tucci wrote: > From the technical perspective, In the best case this seems to belong to > base/process_metrics.cc, which already does lot of computation for memory stuff > and at that point would be great to refactor malloc_dump_provider to use that. > > Taking a step back and going to the general problem, I wonder if we should have > some more general discussion on what is the right metric for whatever the change > will be in M61. > It can be quite easy to accidentally come up with a metric that represents > something that won't make a difference for end users. > Concrete exmaples: what if you measure memory before and after, but then few > seconds later, that memory gets fully reacquired (or worse, there is some bug > and some code ends up acquiring 2x the original memory?) I've written what I am planning in the Metrics section of the design doc: https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... Basically this change is adding a GC triggering signal and so memory reduction is guaranteed. We already have a good idea on the amount of memory reduction with system_health benchmarks. The UMA I plan to collect is not to show how effective the change was, and more about collecting the amount of guaranteed garbage on real world page navigations and use that data to design the future GC scheduling parameters.
On 2017/06/09 04:27:58, keishi wrote: > On 2017/06/08 14:41:21, Primiano Tucci wrote: > > From the technical perspective, In the best case this seems to belong to > > base/process_metrics.cc, which already does lot of computation for memory > stuff > > and at that point would be great to refactor malloc_dump_provider to use that. > > > > Taking a step back and going to the general problem, I wonder if we should > have > > some more general discussion on what is the right metric for whatever the > change > > will be in M61. > > It can be quite easy to accidentally come up with a metric that represents > > something that won't make a difference for end users. > > Concrete exmaples: what if you measure memory before and after, but then few > > seconds later, that memory gets fully reacquired (or worse, there is some bug > > and some code ends up acquiring 2x the original memory?) > > I've written what I am planning in the Metrics section of the design doc: > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > Basically this change is adding a GC triggering signal and so memory reduction > is guaranteed. > We already have a good idea on the amount of memory reduction with system_health > benchmarks. > > The UMA I plan to collect is not to show how effective the change was, and more > about collecting the amount of guaranteed garbage on real world page navigations > and use that data to design the future GC scheduling parameters. My suggestion is: - We must land the GC change by M61 for Android Go. - If memory UMA is available, we should just use it. - Otherwise, we'll need some tentative metric to evaluate the impact.
On 2017/06/09 04:36:24, haraken wrote: > On 2017/06/09 04:27:58, keishi wrote: > > On 2017/06/08 14:41:21, Primiano Tucci wrote: > > > From the technical perspective, In the best case this seems to belong to > > > base/process_metrics.cc, which already does lot of computation for memory > > stuff > > > and at that point would be great to refactor malloc_dump_provider to use > that. > > > > > > Taking a step back and going to the general problem, I wonder if we should > > have > > > some more general discussion on what is the right metric for whatever the > > change > > > will be in M61. > > > It can be quite easy to accidentally come up with a metric that represents > > > something that won't make a difference for end users. > > > Concrete exmaples: what if you measure memory before and after, but then > few > > > seconds later, that memory gets fully reacquired (or worse, there is some > bug > > > and some code ends up acquiring 2x the original memory?) > > > > I've written what I am planning in the Metrics section of the design doc: > > > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > > > Basically this change is adding a GC triggering signal and so memory reduction > > is guaranteed. > > We already have a good idea on the amount of memory reduction with > system_health > > benchmarks. > > > > The UMA I plan to collect is not to show how effective the change was, and > more > > about collecting the amount of guaranteed garbage on real world page > navigations > > and use that data to design the future GC scheduling parameters. > > My suggestion is: > > - We must land the GC change by M61 for Android Go. > - If memory UMA is available, we should just use it. > - Otherwise, we'll need some tentative metric to evaluate the impact. Alright. Can we move it at least to base/process/process_metrics.h so at least it's in the same place that measures other raw memory stuff? Ideally we can first move the functions as they are, and then in a 2nd cl refactor the malloc stuff with malloc_dump_provider (at that point the two files seem to contain 90% of the same code).
On 2017/06/09 13:16:55, Primiano Tucci wrote: > On 2017/06/09 04:36:24, haraken wrote: > > On 2017/06/09 04:27:58, keishi wrote: > > > On 2017/06/08 14:41:21, Primiano Tucci wrote: > > > > From the technical perspective, In the best case this seems to belong to > > > > base/process_metrics.cc, which already does lot of computation for memory > > > stuff > > > > and at that point would be great to refactor malloc_dump_provider to use > > that. > > > > > > > > Taking a step back and going to the general problem, I wonder if we should > > > have > > > > some more general discussion on what is the right metric for whatever the > > > change > > > > will be in M61. > > > > It can be quite easy to accidentally come up with a metric that represents > > > > something that won't make a difference for end users. > > > > Concrete exmaples: what if you measure memory before and after, but then > > few > > > > seconds later, that memory gets fully reacquired (or worse, there is some > > bug > > > > and some code ends up acquiring 2x the original memory?) > > > > > > I've written what I am planning in the Metrics section of the design doc: > > > > > > https://docs.google.com/document/d/1vJKPuMbJ5CQiFwrkxvTrB-x25SKF22-ui42oExvwp... > > > > > > Basically this change is adding a GC triggering signal and so memory > reduction > > > is guaranteed. > > > We already have a good idea on the amount of memory reduction with > > system_health > > > benchmarks. > > > > > > The UMA I plan to collect is not to show how effective the change was, and > > more > > > about collecting the amount of guaranteed garbage on real world page > > navigations > > > and use that data to design the future GC scheduling parameters. > > > > My suggestion is: > > > > - We must land the GC change by M61 for Android Go. > > - If memory UMA is available, we should just use it. > > - Otherwise, we'll need some tentative metric to evaluate the impact. > > Alright. Can we move it at least to base/process/process_metrics.h so at least > it's in the same place that measures other raw memory stuff? Ideally we can > first move the functions as they are, and then in a 2nd cl refactor the malloc > stuff with malloc_dump_provider (at that point the two files seem to contain 90% > of the same code). OK. Thanks! process_metrics feels like the right place. Updated CL. PTAL
keishi@chromium.org changed reviewers: + mark@chromium.org
+mark Could you review base/? Thanks.
https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... File base/process/process_metrics.cc (right): https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.cc:14: #if defined(OS_MACOSX) looks like you could split this file in process_metrics_win.cc and process_metrics_posix.cc instead of putting in the generic .cc and then #ifdef https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.h:547: BASE_EXPORT size_t GetMallocUsage(); I defer this to mark@ but just pointing out that the other functions here seem to be member functions. I'd probably make them such just for consistency even if I realize that there is no functional requirement. I think this all comes from the fact that one can get a ProcessMetrics for another process. But then if you look at the actual code, most of these functions don't even look at process_; and always assume that proces_ == local.
https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... File base/process/process_metrics.cc (right): https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.cc:172: LightPartitionStatsDumperImpl dumper; also, looks like you should call PartitionDumpTotals(...) on the various partitions to actually accumulate the total? this will just always return 0 as it is
https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... File base/process/process_metrics.cc (right): https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.cc:14: #if defined(OS_MACOSX) On 2017/06/14 12:10:32, Primiano Tucci wrote: > looks like you could split this file in process_metrics_win.cc and > process_metrics_posix.cc instead of putting in the generic .cc and then #ifdef Done. https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.cc:172: LightPartitionStatsDumperImpl dumper; On 2017/06/14 12:19:49, Primiano Tucci wrote: > also, looks like you should call PartitionDumpTotals(...) on the various > partitions to actually accumulate the total? this will just always return 0 as > it is I forgot to call WTF::Partitions::DumpMemoryStats(). But since it contains Blink specific partition names I think I'll give up moving GetPartitionAllocUsage into base. It wasn't necessary, I just thought it would be nice to have it next to GetMallocUsage. https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2925073002/diff/20001/base/process/process_me... base/process/process_metrics.h:547: BASE_EXPORT size_t GetMallocUsage(); On 2017/06/14 12:10:32, Primiano Tucci wrote: > I defer this to mark@ but just pointing out that the other functions here seem > to be member functions. I'd probably make them such just for consistency even if > I realize that there is no functional requirement. > > I think this all comes from the fact that one can get a ProcessMetrics for > another process. But then if you look at the actual code, most of these > functions don't even look at process_; and always assume that proces_ == local. Done.
Cool, non-owner LGTM. plz update your CL title as it still mentions partition_alloc
LGTM in base.
Description was changed from ========== Move malloc/partition_alloc memory usage functions to base Move malloc/partition_alloc memory usage functions to base so they can be used inside Blink as well as in content/ BUG=None ========== to ========== Move malloc memory usage function to base Move malloc memory usage function to base so they can be used inside Blink as well as in content/ BUG=None ==========
keishi@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko could you review the change to content/renderer/render_thread_impl.cc? Thanks.
lgtm
The CQ bit was checked by keishi@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...
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 keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tasak@google.com Link to the patchset: https://codereview.chromium.org/2925073002/#ps40001 (title: "Moved to ProcessMetrics method, etc")
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": 1497934467786890,
"parent_rev": "4ae8d1757f7b5a0cb85497205a80ed2a572a5165", "commit_rev":
"12b598b94796f25a6207e89eb844d2c73145858e"}
Message was sent while issue was closed.
Description was changed from ========== Move malloc memory usage function to base Move malloc memory usage function to base so they can be used inside Blink as well as in content/ BUG=None ========== to ========== Move malloc memory usage function to base Move malloc memory usage function to base so they can be used inside Blink as well as in content/ BUG=None Review-Url: https://codereview.chromium.org/2925073002 Cr-Commit-Position: refs/heads/master@{#480784} Committed: https://chromium.googlesource.com/chromium/src/+/12b598b94796f25a6207e89eb844... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/12b598b94796f25a6207e89eb844... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
