[Durable Messages] Report aggregate memory usage via UMA [chromium/src : main]
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
void DevtoolsDurableMessageCollectorManager::ReportAggregateMemoryUsage() {Alex N. JoseHave you considered using the memory-infra machinery for reporting memory usage?
A benefit from that is that you get the same sampling rate (random intervals, with a constant mean) as other memory histograms, and you don't have to care about posting tasks, etc.
Alex N. JoseNo I haven't, and I couldn't find much information about it. Can you point me in the right direction?
Benoit Lize@li...@chromium.org, bumping this question. I can also follow up on a separate CL to refine the collection of metrics, if this is currently in a shape that's good to land as is.
Alex N. JoseIt requires a few steps:
- Inherit from MemoryDumpProvider, either in the the class you are changing, or in a new one
- Register this instance to provide data
- Then, in the place that collects the data, add the mapping between the metric you want to report and a UMA name
If you want a recent-ish example, you can look at this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/6624543
In this case, there is already a memory dump provider in the class, so the first step had been done in the past. However, the rest applies, that is:
- a memory dump is (roughly) like a JSON dict where things can be added
- They are found in the browser process by looking at their "path"
One thing to note: since you want this to power UMA metrics collection, you need to add your new dump provider to the background allowlist in base/trace_event/memory_infra_background_allowlist.cc
What you get in return for all this trouble is that your metrics are sampled at a random interval with a mean of 5 minutes on Android, 30 on desktop, and at the same time as the other metrics.
Thanks for the guidance, @li...@chromium.org, I've updated the code to use MemoryDumpProvider. PTAL!
Related details
- Andrey Kosyakov
- Benoit Lize
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize added 3 comments![]( "Open in Gerrit")
"NetworkService.DurableMessages.AggregateMemoryUsage", MetricSize::kLarge,Do you expect the sizes to be larger than 500MiB, and are OK with not having the first bucket be 0-1MiB?
const auto& entries = dump->entries();This code can be simplified, like in:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/parkable_string_test.cc;drc=c4b3eb4bc4fe15ed95acb76c9c68e4d056fab614;l=1084
(And other similar locations)
<histogram name="Memory.NetworkService.DurableMessages.AggregateMemoryUsage"Have you checked about:histograms?
I don't think this is the histogram name that will be emitted.
Histogram names are the concatenation of several parts, and you should put it as a suffix.
Related details
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose added 5 comments![]( "Open in Gerrit")
Alex N. JoseAs with mmenke, I think tracking of memory usage & tests look fine, waiting to learn about memory-infra.
If we end up not switching to memory-infra, we should probably think a bit more about whether we're collecting too many samples and whether this sampling method introduces bias.
Done
"NetworkService.DurableMessages.AggregateMemoryUsage", MetricSize::kLarge,Do you expect the sizes to be larger than 500MiB, and are OK with not having the first bucket be 0-1MiB?
Yes, given this would account for the aggregate amount of memory collected (which are today across several renderers) across the page loads, there is no reason to look for granularity within the first MiB. Sizes may run over 500 MiB.
This code can be simplified, like in:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/parkable_string_test.cc;drc=c4b3eb4bc4fe15ed95acb76c9c68e4d056fab614;l=1084(And other similar locations)
Done
<histogram name="Memory.NetworkService.DurableMessages.AggregateMemoryUsage"Have you checked about:histograms?
I don't think this is the histogram name that will be emitted.
Histogram names are the concatenation of several parts, and you should put it as a suffix.
It's getting emitted in about:histograms as ```
- Histogram: Memory.Experimental.NetworkService2.NetworkService.DurableMessages.AggregateMemoryUsage recorded 2 samples, mean = 7.0 (flags = 0x41) [#]
- 0 ...
- 7 --O (2 = 100.0%) {0.0%}
- 8 ...
- ```
The name is different than what's given, the `Memory.` portion seems replaced with `Memory.Experimental.NetworkService2`.
I've moved this into the variants section, [PTAL](chrome://histograms/#Memory.Experimental.NetworkService2.DurableMessages.AggregateMemoryUsage). Would I be able to monitor this value during a Finch experiment? What is the difference between variant vs. a histogram like `Memory.NetworkService.PrivateMemoryFootprint` (which is what I was going after earlier).
Related details
- Adam Rice
- Andrey Kosyakov
- Benoit Lize
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benoit Lize (Gerrit)
Benoit Lize voted and added 3 comments![]( "Open in Gerrit")
Votes added by Benoit Lize
| Code-Review | +1 |
3 comments
"NetworkService.DurableMessages.AggregateMemoryUsage", MetricSize::kLarge,Alex N. JoseDo you expect the sizes to be larger than 500MiB, and are OK with not having the first bucket be 0-1MiB?
Yes, given this would account for the aggregate amount of memory collected (which are today across several renderers) across the page loads, there is no reason to look for granularity within the first MiB. Sizes may run over 500 MiB.
Acknowledged
<histogram name="Memory.NetworkService.DurableMessages.AggregateMemoryUsage"Alex N. JoseHave you checked about:histograms?
I don't think this is the histogram name that will be emitted.
Histogram names are the concatenation of several parts, and you should put it as a suffix.
It's getting emitted in about:histograms as ```
- Histogram: Memory.Experimental.NetworkService2.NetworkService.DurableMessages.AggregateMemoryUsage recorded 2 samples, mean = 7.0 (flags = 0x41) [#]
- 0 ...
- 7 --O (2 = 100.0%) {0.0%}
- 8 ...
- ```
The name is different than what's given, the `Memory.` portion seems replaced with `Memory.Experimental.NetworkService2`.
I've moved this into the variants section, [PTAL](chrome://histograms/#Memory.Experimental.NetworkService2.DurableMessages.AggregateMemoryUsage). Would I be able to monitor this value during a Finch experiment? What is the difference between variant vs. a histogram like `Memory.NetworkService.PrivateMemoryFootprint` (which is what I was going after earlier).
The PrivateMemoryFootprint histogram is recorded using a slightly different path. The new way is the correct one, and yes, this is a regular histogram, that you can monitor.
Related details
- Adam Rice
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex N. Jose
| Auto-Submit | +1 |
1 comment
@mme...@google.com, @ri...@chromium.org, let me know if this is good to land!
Related details
- Adam Rice
- Andrey Kosyakov
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
void DevtoolsDurableMessageCollectorManager::ReportAggregateMemoryUsage() {Have you considered using the memory-infra machinery for reporting memory usage?
A benefit from that is that you get the same sampling rate (random intervals, with a constant mean) as other memory histograms, and you don't have to care about posting tasks, etc.
Alex N. JoseNo I haven't, and I couldn't find much information about it. Can you point me in the right direction?
Benoit Lize@li...@chromium.org, bumping this question. I can also follow up on a separate CL to refine the collection of metrics, if this is currently in a shape that's good to land as is.
Alex N. JoseIt requires a few steps:
- Inherit from MemoryDumpProvider, either in the the class you are changing, or in a new one
- Register this instance to provide data
- Then, in the place that collects the data, add the mapping between the metric you want to report and a UMA name
If you want a recent-ish example, you can look at this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/6624543
In this case, there is already a memory dump provider in the class, so the first step had been done in the past. However, the rest applies, that is:
- a memory dump is (roughly) like a JSON dict where things can be added
- They are found in the browser process by looking at their "path"
One thing to note: since you want this to power UMA metrics collection, you need to add your new dump provider to the background allowlist in base/trace_event/memory_infra_background_allowlist.cc
What you get in return for all this trouble is that your metrics are sampled at a random interval with a mean of 5 minutes on Android, 30 on desktop, and at the same time as the other metrics.
Thanks for the guidance, @li...@chromium.org, I've updated the code to use MemoryDumpProvider. PTAL!
Done
Related details
- Adam Rice
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 4 comments![]( "Open in Gerrit")
Some nits, but otherwise looks fine. Sorry for the day, it wasn't in my attention set. I think I removed it from my attention set when you were talking about the memory infra, since I couldn't really take action until that was addressed.
void TearDown() override { RunUntilCollectorsEmpty(); }Either add `public:` before this or move it down to the protected section.
protected:
base::test::TaskEnvironment task_environment_;Put this down in the private section, before everything else.
If you need to make this protected rather than private, either just remove the protected section, and put all members down at the bottom together, or add accessors.
DevtoolsDurableMessageCollectorManager manager_;As above, either make this private and add an accessor (`DevtoolsDurableMessageCollectorManager& manager()`) or make all values protected, removing the private section, and put them at the bottom.
This is the general way we tend to do things in net/ and services/network - everything private, or everything protected. Since declaration order affects initialization order, and we don't want multiple protected/private sections, best to keep all members in the same section.
Related details
- Adam Rice
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
WaitForEventCount(2);
collector_remote.reset();Also, these aren't needed, are they? it will be reset when it leaves scope, anyways. Or is it important that it be destroyed before msg1/msg2 for some reason?
Alex N. Jose (Gerrit)
Alex N. Jose voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex N. Jose
| Auto-Submit | +1 |
4 comments
Either add `public:` before this or move it down to the protected section.
Done
Put this down in the private section, before everything else.
If you need to make this protected rather than private, either just remove the protected section, and put all members down at the bottom together, or add accessors.
Done
As above, either make this private and add an accessor (`DevtoolsDurableMessageCollectorManager& manager()`) or make all values protected, removing the private section, and put them at the bottom.
This is the general way we tend to do things in net/ and services/network - everything private, or everything protected. Since declaration order affects initialization order, and we don't want multiple protected/private sections, best to keep all members in the same section.
Ack, updated code to reflect it.
Also, these aren't needed, are they? it will be reset when it leaves scope, anyways. Or is it important that it be destroyed before msg1/msg2 for some reason?
- Adam Rice
- Andrey Kosyakov
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke voted and added 1 comment![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
1 comment
LGTM. Note that you don't have full owners coverage, so I'm not submitting (also, Adam's still a reviewer)
Related details
- Adam Rice
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
@dch...@chromium.org, could I get your review of base/trace_event/* and chrome/browser/metrics/* changes? Thanks!
Related details
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex N. Jose
| Auto-Submit | +0 |
1 comment
LGTM. Note that you don't have full owners coverage, so I'm not submitting (also, Adam's still a reviewer)
Thanks! Not sure what happened there, perhaps ownership changed over the lifetime of this CL. Updated reviewers list.
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From analysis/uma/chrome-metrics.gwsq:
Histograms should by default be reviewed by the owners of the subdirectories. The chromium-met...@google.com gwsq should be used when there are no individual owners, or for escalation to the Metrics team.
If you are interested in becoming a metrics reviewer, please review the instructions at https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#Becoming-a-Metrics-Reviewer
Reviewer source(s):
rog...@chromium.org is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Andrey Kosyakov
- Daniel Cheng
- Roger McFarlane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
- Andrey Kosyakov
- Etienne Pierre-Doray
- Roger McFarlane
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
1 comment
- Alex N. Jose
- Andrey Kosyakov
- Roger McFarlane
Roger McFarlane (Gerrit)
Roger McFarlane voted and added 1 comment![]( "Open in Gerrit")
Votes added by Roger McFarlane
| Code-Review | +1 |
1 comment
total_memory_usage_ += size;is it worth asserting that all of these methods run on the same sequence?
Related details
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
total_memory_usage_ += size;is it worth asserting that all of these methods run on the same sequence?
The network service is basically single threaded. I'm fine with adding them, but I don't think there's much value there.
Roger McFarlane (Gerrit)
Roger McFarlane added 1 comment![]( "Open in Gerrit")
total_memory_usage_ += size;is it worth asserting that all of these methods run on the same sequence?
I'm referring to adding a sequence checker and validating that these methods are not called concurrently.
https://source.chromium.org/chromium/chromium/src/+/main:base/sequence_checker.h
Roger McFarlane (Gerrit)
Roger McFarlane added 1 comment![]( "Open in Gerrit")
total_memory_usage_ += size;Roger McFarlaneis it worth asserting that all of these methods run on the same sequence?
I'm referring to adding a sequence checker and validating that these methods are not called concurrently.
https://source.chromium.org/chromium/chromium/src/+/main:base/sequence_checker.h
Ack
Related details
- Alex N. Jose
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roger McFarlane (Gerrit)
Roger McFarlane added 1 comment![]( "Open in Gerrit")
total_memory_usage_ += size;Roger McFarlaneis it worth asserting that all of these methods run on the same sequence?
Roger McFarlaneI'm referring to adding a sequence checker and validating that these methods are not called concurrently.
https://source.chromium.org/chromium/chromium/src/+/main:base/sequence_checker.h
Ack
Comment in the middle was for alexnj@ who messaged me to ask for clarification.
Alex N. Jose (Gerrit)
Alex N. Jose voted Commit-Queue+2![]( "Open in Gerrit")
- Andrey Kosyakov
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Durable Messages] Report aggregate memory usage via UMA
To account for the memory used by DevTools Durable Messages within
Network Service, this CL implements a UMA that reports the global
memory usage by all collected messages.
Note that this doesn't limit the total memory used, to retain
current DevTools capability as is.
- M base/trace_event/memory_infra_background_allowlist.cc
- M chrome/browser/metrics/process_memory_metrics_emitter.cc
- M services/network/devtools_durable_msg_collector.cc
- M services/network/devtools_durable_msg_collector_manager.cc
- M services/network/devtools_durable_msg_collector_manager.h
- M services/network/devtools_durable_msg_collector_manager_unittest.cc
- M services/network/devtools_durable_msg_collector_unittest.cc
- M tools/metrics/histograms/metadata/memory/histograms.xml
Code-Review: +1 by mmenke, +1 by Etienne Pierre-Doray, +1 by Benoit Lize, +1 by Roger McFarlane
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |