Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2824133002:
Record swap metrics (Closed)
DescriptionRecord swap metrics
This CL adds SwapMetricsObserver which records swap rates and reports
them as platform-specific metrics. The purpose of adding these metrics
is to measure the impact of actions we take to free up memory. If we
aggressively free up memory under high memory pressure, we may touch
pages which are already swapped out (or compressed). In such cases
thrashing could happen and we are making the situation worse.
Swap rates and page compression/decompression rate would be good
indicators of thrashing. Metrics will be recorded every 60 seconds.
This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs
will add them for macOS.
BUG=617492
Review-Url: https://codereview.chromium.org/2824133002
Cr-Commit-Position: refs/heads/master@{#468266}
Committed: https://chromium.googlesource.com/chromium/src/+/4159f36e93c398b9816b734a14e97220e83fc912
Patch Set 1 #
Total comments: 11
Patch Set 2 : comments #
Total comments: 4
Patch Set 3 : comments #
Total comments: 2
Messages
Total messages: 35 (18 generated)
The CQ bit was checked by bashi@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...
Description was changed from ========== Record swap metrics I'll update the description later. BUG=617492 ========== to ========== Record swap metrics This CL adds SwapMetricsObserver which records swap rates and reports them as platform-specific metrics. The purpose of adding these metrics is to measure the impact of actions we take to free up memory. If we aggressively free up memory under high memory pressure, we may touch pages which are already swapped out (or compressed). In such cases we just make the situation worse. Swap rates and page compression/decompression rate would be good indicators of thrashing. This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs will add them for macOS. Metrics will be recorded every 60 seconds. BUG=617492 ==========
bashi@chromium.org changed reviewers: + chrisha@chromium.org, haraken@chromium.org
I tried some ideas to record swapping and page compression behavior and ended up using this simple timer approach. What do you think? Caveats: - We have Memory.Swap.* metrics but they are not useful. They are cumulative and don't represent how busy the system is for handling memory pressure. - Metrics should be independent from memory coordinator so that we can compare the metrics w/ and w/o memory coordinator. https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_win.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_win.cc:11: // SwapMetricsObserver isn't available on Windows. I couldn't find any API to get swap rates and/or (de)compression rates on Windows.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@chromium.org changed reviewers: + erikchen@chromium.org, primiano@chromium.org, shrike@chromium.org
I might want to get some OS experts to review this CL. shrike@, primiano@, erikchen@: Would you take a look at this one?
https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_mac.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_mac.cc:11: // TODO(bashi): Implement SwapMetricsObserver for macOS. You'll want to use host_statistics64. See xnu-3789.1.32/osfmk/kern/host.c:524, available at https://opensource.apple.com/release/macos-1012.html Relevant stats: 607 stat->compressions = host_vm_stat.compressions; 608 stat->decompressions = host_vm_stat.decompressions; 609 stat->swapins = host_vm_stat.swapins; 610 stat->swapouts = host_vm_stat.swapouts; ... 618 stat->total_uncompressed_pages_in_compressor = c_segment_pages_compressed; Keep in mind that macOS uses both a compressor and swap. The basic structure is: resident memory <--can be compressed/decompressed--> compressed memory . <--can be swapped/unswapped--> swap
Any feedback? I guess that experts have better ideas :) https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_mac.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_mac.cc:11: // TODO(bashi): Implement SwapMetricsObserver for macOS. On 2017/04/18 17:48:19, erikchen wrote: > You'll want to use host_statistics64. > > See xnu-3789.1.32/osfmk/kern/host.c:524, available at > https://opensource.apple.com/release/macos-1012.html > > Relevant stats: > > 607 stat->compressions = host_vm_stat.compressions; > > 608 stat->decompressions = host_vm_stat.decompressions; > > 609 stat->swapins = host_vm_stat.swapins; > > 610 stat->swapouts = host_vm_stat.swapouts; > > ... > 618 stat->total_uncompressed_pages_in_compressor = > c_segment_pages_compressed; > > Keep in mind that macOS uses both a compressor and swap. The basic structure is: > resident memory <--can be compressed/decompressed--> compressed memory . <--can > be swapped/unswapped--> swap Thanks for detailed info! Using host_statistics64 is the plan and I'm aware of macOS uses both compression and swap. I'm planning to implement it in follow-up CLs if the overall approach is reasonable.
Description was changed from ========== Record swap metrics This CL adds SwapMetricsObserver which records swap rates and reports them as platform-specific metrics. The purpose of adding these metrics is to measure the impact of actions we take to free up memory. If we aggressively free up memory under high memory pressure, we may touch pages which are already swapped out (or compressed). In such cases we just make the situation worse. Swap rates and page compression/decompression rate would be good indicators of thrashing. This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs will add them for macOS. Metrics will be recorded every 60 seconds. BUG=617492 ========== to ========== Record swap metrics This CL adds SwapMetricsObserver which records swap rates and reports them as platform-specific metrics. The purpose of adding these metrics is to measure the impact of actions we take to free up memory. If we aggressively free up memory under high memory pressure, we may touch pages which are already swapped out (or compressed). In such cases thrashing could happen and we are making the situation worse. Swap rates and page compression/decompression rate would be good indicators of thrashing. Metrics will be recorded every 60 seconds. This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs will add them for macOS. BUG=617492 ==========
Hi reviewers, Can I proceed with this approach to measure swap rates? Or should GRC collect these metrics? How soon will GRC be available?
On 2017/04/27 04:36:45, bashi wrote: > Hi reviewers, > > Can I proceed with this approach to measure swap rates? Or should GRC collect > these metrics? How soon will GRC be available? GRC won't be available soon. Let's just make sure that we're measuring the swap rate correctly and land this CL. primiano@: Would you take a look at *_linux.cc?
content/browser/memory/swap_metrics_observer_linux.* LGTM Did you try if this works on Android (just wondering if reading /proc/vmstat is allowed by selinux under M+) ? https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_linux.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.cc:19: return base::Singleton< IWYU (+base/singleton.h). BTW these days you can just use static locals, as they are thread-safe in c++11, i..e static SwapMemoryObserverLinux* instance = new SwapMemoryObserverLinux(); return instance. The alternative I learned from dcheng@ is using base::LazyInstance, which is more efficient: it doesnt cause a new (conversely to both singleton and the static local) and allocates directly the instance from static storage in bzz, hence reducing the indirection layers involved. https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.cc:35: return; maybe also stop the timer in these failure cases? https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_linux.h (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.h:14: static SwapMetricsObserverLinux* GetInstance(); you don't seem to make any use of this specialized GetInstance, other than using that in the general GetInstance that returns just a SMO. Can you just use and return the singleton directly in the SwapMetricsObserver* SwapMetricsObserver::GetInstance() implementation? https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_win.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_win.cc:11: // SwapMetricsObserver isn't available on Windows. On 2017/04/18 07:12:33, bashi wrote: > I couldn't find any API to get swap rates and/or (de)compression rates on > Windows. There is a PageFaultsCount, PROCESS_MEMORY_COUNTERS_EX although I suspect that would count soft + hard, while you want only hard here? An alternative would be using WMI, but that is a huge pain and not sure it's worth it here. I'd ask chrisha@ / brucedawson@
Thanks Primiano for review. > Did you try if this works on Android (just wondering if reading /proc/vmstat is > allowed by selinux under M+) ? I tried on my Nexus5X (7.1.2) and reading /proc/vmstat seems working even selinux is enabled. https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_linux.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.cc:19: return base::Singleton< On 2017/04/27 10:01:20, Primiano Tucci wrote: > IWYU (+base/singleton.h). > BTW these days you can just use static locals, as they are thread-safe in c++11, > i..e > > static SwapMemoryObserverLinux* instance = new SwapMemoryObserverLinux(); > return instance. > > The alternative I learned from dcheng@ is using base::LazyInstance, which is > more efficient: it doesnt cause a new (conversely to both singleton and the > static local) and allocates directly the instance from static storage in bzz, > hence reducing the indirection layers involved. Changed to use static local. Thanks for the info! https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.cc:35: return; On 2017/04/27 10:01:20, Primiano Tucci wrote: > maybe also stop the timer in these failure cases? Yes. Probably we shouldn't start the timer in the first place when these fails. Added checks. https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_linux.h (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_linux.h:14: static SwapMetricsObserverLinux* GetInstance(); On 2017/04/27 10:01:20, Primiano Tucci wrote: > you don't seem to make any use of this specialized GetInstance, other than using > that in the general GetInstance that returns just a SMO. Can you just use and > return the singleton directly in the SwapMetricsObserver* > SwapMetricsObserver::GetInstance() > implementation? Done. https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... File content/browser/memory/swap_metrics_observer_win.cc (right): https://codereview.chromium.org/2824133002/diff/1/content/browser/memory/swap... content/browser/memory/swap_metrics_observer_win.cc:11: // SwapMetricsObserver isn't available on Windows. On 2017/04/27 10:01:20, Primiano Tucci wrote: > On 2017/04/18 07:12:33, bashi wrote: > > I couldn't find any API to get swap rates and/or (de)compression rates on > > Windows. > > There is a PageFaultsCount, PROCESS_MEMORY_COUNTERS_EX although I suspect that > would count soft + hard, while you want only hard here? > An alternative would be using WMI, but that is a huge pain and not sure it's > worth it here. > I'd ask chrisha@ / brucedawson@ Yeah, I thought PageFaultsCount may be noisy as it may count soft + hard. Also I think that hard page faults would contain demand paging which doesn't necessary to be considered as swap rates IMO. I added a TODO comment to seek whether we can add swap rates on Windows.
LGTM
bashi@chromium.org changed reviewers: + isherman@chromium.org, jam@chromium.org
jam@: could you review content/browser? isherman@: could you review histograms.xml?
On 2017/04/28 05:31:45, bashi wrote: > jam@: could you review content/browser? > isherman@: could you review histograms.xml? lgtm (didn't review content/browser/memory since you're owner there)
Metrics LGTM % nits: https://codereview.chromium.org/2824133002/diff/20001/content/browser/memory/... File content/browser/memory/swap_metrics_observer.cc (right): https://codereview.chromium.org/2824133002/diff/20001/content/browser/memory/... content/browser/memory/swap_metrics_observer.cc:16: } nit: Please leave a blank line before this closing brace, and add " // namespace" to the end of this line. https://codereview.chromium.org/2824133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29391: +<histogram name="Memory.Experimental.SwapInPerSecond" units="counts/s"> nit: Would units="swaps/s" be accurate? If so, I think it'd be a bit clearer.
The CQ bit was checked by bashi@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.
https://codereview.chromium.org/2824133002/diff/20001/content/browser/memory/... File content/browser/memory/swap_metrics_observer.cc (right): https://codereview.chromium.org/2824133002/diff/20001/content/browser/memory/... content/browser/memory/swap_metrics_observer.cc:16: } On 2017/04/28 21:10:20, Ilya Sherman wrote: > nit: Please leave a blank line before this closing brace, and add " // > namespace" to the end of this line. Done. https://codereview.chromium.org/2824133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2824133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29391: +<histogram name="Memory.Experimental.SwapInPerSecond" units="counts/s"> On 2017/04/28 21:10:20, Ilya Sherman wrote: > nit: Would units="swaps/s" be accurate? If so, I think it'd be a bit clearer. Done.
The CQ bit was checked by bashi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, jam@chromium.org, haraken@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2824133002/#ps40001 (title: "comments")
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": 1493601265449660,
"parent_rev": "96988faed343f078f05d0f364ce5de260d068be0", "commit_rev":
"4159f36e93c398b9816b734a14e97220e83fc912"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493601265449660,
"parent_rev": "96988faed343f078f05d0f364ce5de260d068be0", "commit_rev":
"4159f36e93c398b9816b734a14e97220e83fc912"}
Message was sent while issue was closed.
Description was changed from ========== Record swap metrics This CL adds SwapMetricsObserver which records swap rates and reports them as platform-specific metrics. The purpose of adding these metrics is to measure the impact of actions we take to free up memory. If we aggressively free up memory under high memory pressure, we may touch pages which are already swapped out (or compressed). In such cases thrashing could happen and we are making the situation worse. Swap rates and page compression/decompression rate would be good indicators of thrashing. Metrics will be recorded every 60 seconds. This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs will add them for macOS. BUG=617492 ========== to ========== Record swap metrics This CL adds SwapMetricsObserver which records swap rates and reports them as platform-specific metrics. The purpose of adding these metrics is to measure the impact of actions we take to free up memory. If we aggressively free up memory under high memory pressure, we may touch pages which are already swapped out (or compressed). In such cases thrashing could happen and we are making the situation worse. Swap rates and page compression/decompression rate would be good indicators of thrashing. Metrics will be recorded every 60 seconds. This CL only adds metrics for Android/ChromeOS/Linux. Follow-up CLs will add them for macOS. BUG=617492 Review-Url: https://codereview.chromium.org/2824133002 Cr-Commit-Position: refs/heads/master@{#468266} Committed: https://chromium.googlesource.com/chromium/src/+/4159f36e93c398b9816b734a14e9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4159f36e93c398b9816b734a14e9...
Message was sent while issue was closed.
https://codereview.chromium.org/2824133002/diff/40001/content/browser/memory/... File content/browser/memory/swap_metrics_observer_win.cc (right): https://codereview.chromium.org/2824133002/diff/40001/content/browser/memory/... content/browser/memory/swap_metrics_observer_win.cc:12: // TODO(bashi): Figure out a way to measure swap rates on Windows. You can find detailed stats for a particular process (CoW faults, soft faults, hard faults, file cache faults, etc) via undocumented NT internals: NtQuerySystemInformation and SYSTEM_PERFORMANCE_INFORMATION. We have similar code using this kind of functionality here: https://cs.chromium.org/chromium/src/components/startup_metric_utils/browser/... You can get system wide performance counters in various ways. There are many "undocumented" structures that contain performance data, including: VM_COUNTERS, SYSTEM_FILECACHE_INFORMATION, SYSTEM_MEMORY_USAGE, SYSTEM_PAGEFILE_INFORMATION, SYSTEM_PERFORMANCE_INFORMATION_2 (extremely detailed), SYSTEM_BASIC_PERFORMANCE_INFORMATION, SYSTEM_BASIC_INFORMATION_EX There's also a whole "performance counter" API. This wraps many of the NT internals, and is much slower but much more clearly documented. See PdhOpenQuery, PdhAddCounter, PdhGetCounterInfo, PdhGetFormattedCounterInfo, PdhCloseQuery. Some experimentation needs to be done to figure out which NT internals are the exact counters you want, when comparing them to the clearly documented performance counters. However, in production code I'd use the NT internal APIs directly as they are many many orders of magnitude faster than the performance counters. (On my machine I could saturate a CPU querying the performance counters 1K times per second, where the internal APIs could be hit millions of times per second.) If we're interested in compressed memory pages there are also ways to "estimate" this by tracking the working set size of a particular system process. Ping me directly for more info.
Message was sent while issue was closed.
https://codereview.chromium.org/2824133002/diff/40001/content/browser/memory/... File content/browser/memory/swap_metrics_observer_win.cc (right): https://codereview.chromium.org/2824133002/diff/40001/content/browser/memory/... content/browser/memory/swap_metrics_observer_win.cc:12: // TODO(bashi): Figure out a way to measure swap rates on Windows. On 2017/05/03 15:55:56, chrisha wrote: > You can find detailed stats for a particular process (CoW faults, soft faults, > hard faults, file cache faults, etc) via undocumented NT internals: > NtQuerySystemInformation and SYSTEM_PERFORMANCE_INFORMATION. We have similar > code using this kind of functionality here: > > https://cs.chromium.org/chromium/src/components/startup_metric_utils/browser/... > > You can get system wide performance counters in various ways. There are many > "undocumented" structures that contain performance data, including: > > VM_COUNTERS, SYSTEM_FILECACHE_INFORMATION, SYSTEM_MEMORY_USAGE, > SYSTEM_PAGEFILE_INFORMATION, SYSTEM_PERFORMANCE_INFORMATION_2 (extremely > detailed), SYSTEM_BASIC_PERFORMANCE_INFORMATION, SYSTEM_BASIC_INFORMATION_EX > > There's also a whole "performance counter" API. This wraps many of the NT > internals, and is much slower but much more clearly documented. See > PdhOpenQuery, PdhAddCounter, PdhGetCounterInfo, PdhGetFormattedCounterInfo, > PdhCloseQuery. > > Some experimentation needs to be done to figure out which NT internals are the > exact counters you want, when comparing them to the clearly documented > performance counters. However, in production code I'd use the NT internal APIs > directly as they are many many orders of magnitude faster than the performance > counters. (On my machine I could saturate a CPU querying the performance > counters 1K times per second, where the internal APIs could be hit millions of > times per second.) > > If we're interested in compressed memory pages there are also ways to "estimate" > this by tracking the working set size of a particular system process. Ping me > directly for more info. Thank you Chris for the detailed information. That's really helpful. I glanced over startup_metrics_utils.cc and web sites which are linked from comments in the file. I don't fully understand internal NT APIs and performance counter APIs yet but I'll try to follow your suggestion, i.e. using performance counter APIs to figure out which stats we should track then find corresponding internal ones. |
