Optimize browser in-memory metrics footprint |
||||||
Issue descriptionPer https://docs.google.com/spreadsheets/d/15Ml4abjUrWi2R1D3zwtOAHn573o7ERoWw6IPRqb0xos/edit#gid=445248581, it looks like metrics may account for something like ~2MB in a 64 bit browser. I typically see around 4K PersistentSampleVector (PSV) instances after an hour or two of usage, and the tally above is about 500 histograms short from that. (Note however that the "Owned bytes" is a heuristic, so shouldn't be treated as gospel yet). It's also apparent that there's a large size difference between the 32 and 64 bit PSV instances (72 vs 108 bytes per, IIRC). This can probably be optimized a fair bit by using explicitly sized integer types e.g. here <https://cs.chromium.org/chromium/src/base/metrics/persistent_memory_allocator.h?l=852>, and/or by better member ordering. Also, long term, there's the question whether a sizable fraction of histograms are single or rare use. Apparently ~50% of histograms hit the single bucket optimization, and might well be single use. It's wasteful - possibly significantly so on mobile devices - to hold on to the in-memory representation of such histograms indefinitely. Maybe there's room for optimization there too.
,
Jun 18 2017
The single-value optimization prevents allocation of the "counts" array but the SampleVector object still needs to be created at that time in case that array is needed in the future. When the time comes that the array is needed, information in the SampleVector is needed to created it. There's no easy/fast way at that time to "go the long way" and allocate it. The best way to reduce the size of the SampleVector objects to to remove the unused histograms as that will also reduce the number of other objects, std::map entries, metadata allocations, and more. There are regular bugs filed against the biggest (ab)users of this, though its measured against the amount of log space used on servers rather than bytes of memory in the Chrome client. A quick look at the SampleVector objects and dependents shows 16 bytes that can be saved by reducing size_t to uint32_t since it's known that the held values will never exceed the 32-bit boundary. There's also 24 bytes that can be dropped by splitting HistogramSamples into three classes, one persistent and one not, plus a common base class.
,
Jul 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e72a1554a6b4efb3ea711ead4876f0f3e276e45 commit 4e72a1554a6b4efb3ea711ead4876f0f3e276e45 Author: bcwhite <bcwhite@chromium.org> Date: Tue Jul 04 21:13:15 2017 Reduce memory usage in DelayedPersistentAllocation. The PMA is limited to 32-bit values so it's known that |size| and |offset| can't exceed that. No need to waste space on 64-bit builds. BUG= 733380 Review-Url: https://codereview.chromium.org/2970883002 Cr-Commit-Position: refs/heads/master@{#484158} [modify] https://crrev.com/4e72a1554a6b4efb3ea711ead4876f0f3e276e45/base/metrics/persistent_memory_allocator.cc [modify] https://crrev.com/4e72a1554a6b4efb3ea711ead4876f0f3e276e45/base/metrics/persistent_memory_allocator.h
,
Jul 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b4d87bb384bfc8b43cadc24160c0da01ab09f555 commit b4d87bb384bfc8b43cadc24160c0da01ab09f555 Author: bcwhite <bcwhite@chromium.org> Date: Wed Jul 05 15:28:57 2017 Remove typically unused local_metadata_ field. The vast majority of histograms and their samples are now "persistent" with the memory coming from a special memory segment. It's wasteful to have a "local metadata" structure in each sample object that is never used. Instead, allocate the structure from the heap for only those objects that need it and release it when those objects get destructed. BUG= 733380 Review-Url: https://codereview.chromium.org/2973603002 Cr-Commit-Position: refs/heads/master@{#484282} [modify] https://crrev.com/b4d87bb384bfc8b43cadc24160c0da01ab09f555/base/metrics/histogram_samples.cc [modify] https://crrev.com/b4d87bb384bfc8b43cadc24160c0da01ab09f555/base/metrics/histogram_samples.h [modify] https://crrev.com/b4d87bb384bfc8b43cadc24160c0da01ab09f555/base/metrics/sample_map.cc [modify] https://crrev.com/b4d87bb384bfc8b43cadc24160c0da01ab09f555/base/metrics/sample_vector.cc [modify] https://crrev.com/b4d87bb384bfc8b43cadc24160c0da01ab09f555/base/metrics/sample_vector.h
,
Jul 5 2017
Those two changes should reduce PersistentSampleVector by 24/40 bytes (32/64 bit build) or about 33/40%. They will also reduce PersistentSampleMap by 24 bytes or about 33%.
,
Jul 5 2017
Automatically dropping single-use histograms doesn't seem practical. The metrics system doesn't know if things will be used again or if outside code retains pointers to the histogram objects. It also has to remain at least until reported. However, a flag could be added to single-use histograms that causes them to be forgotten after being reported, thus freeing the memory. This would only affect those histograms that took the trouble to set that flag, so I'm thinking that it's not worth the effort.
,
Jul 5 2017
+mariakhomenko FYI
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1dec483900f65168b0603556c1cb77c2f634864 commit f1dec483900f65168b0603556c1cb77c2f634864 Author: bcwhite <bcwhite@chromium.org> Date: Thu Jul 06 14:39:11 2017 Reduce Histogram object size. 1) Remove |final_delta_created_| on non-DCHECK builds. Though this is only a "bool", it's effectively a native int due to object size rounding requirements. 2) Remove |declared_min_| and |declared_max_| fields which are used only for checks and serialization, the values of which can be determined from the range buckets. These remove 16 bytes from an 88-byte structure on 64-bit, or about 20%. BUG= 733380 Review-Url: https://codereview.chromium.org/2965083002 Cr-Commit-Position: refs/heads/master@{#484585} [modify] https://crrev.com/f1dec483900f65168b0603556c1cb77c2f634864/base/metrics/histogram.cc [modify] https://crrev.com/f1dec483900f65168b0603556c1cb77c2f634864/base/metrics/histogram.h
,
Jul 14 2017
Thanks for trimming things down -- really appreciated. Just FYI, currently we don't ship 64-bit Chrome for Android. However, this is probably helpful to webview, which does run in 64-bit mode.
,
Jul 14 2017
,
Jul 17 2017
For 32-bit builds, removing |final_delta_created_| and |declared_min/max_| will have saved 12 bytes per instance. PersistentSampleMap obviously won't be affected. I've got a last CL in the works that will drop another 4-to-24 bytes (depending upon the implementation of std::string used by Android) from every Histogram object plus eliminate duplicates of every histogram name string when persistence is enabled (the default), say roughly an additional 64 bytes per histogram. This is independent of 32/64 bit builds. https://codereview.chromium.org/2973863002/
,
Jul 17 2017
Very nice! Thanks for all your work. Would be really great if you can land before Thursday.
,
Jul 17 2017
Brian is on vacation this week and next, so I think it won't land before branch point. But given that it will land shortly after branch point, we could see if we could get it merged back if it's important for low mem.
,
Jul 17 2017
Maybe we can take a look at the impact on our graphs once it lands and see whether we can justify the merge into M-61.
,
Jul 18 2017
Perhaps. It's no longer as invasive a change as it was originally so it might be okay to submit it for M61.
,
Jul 28 2017
Pinging this bug. I don't think we need to merge to M-61, but would be nice to land regardless.
,
Aug 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32052d5b2d3be4ac84b875e250158e2b6b773e15 commit 32052d5b2d3be4ac84b875e250158e2b6b773e15 Author: Brian White <bcwhite@chromium.org> Date: Mon Aug 07 16:46:57 2017 Remove BucketRanges pointer from Histogram that is duplicated in held object. Bug: 733380 Change-Id: I64593a71af73fc06c38184b918bc9840ad71e2d1 Reviewed-on: https://chromium-review.googlesource.com/603528 Reviewed-by: Alexei Svitkine (OOO July28-Aug6) <asvitkine@chromium.org> Commit-Queue: Brian White <bcwhite@chromium.org> Cr-Commit-Position: refs/heads/master@{#492339} [modify] https://crrev.com/32052d5b2d3be4ac84b875e250158e2b6b773e15/base/metrics/histogram.cc [modify] https://crrev.com/32052d5b2d3be4ac84b875e250158e2b6b773e15/base/metrics/histogram.h [modify] https://crrev.com/32052d5b2d3be4ac84b875e250158e2b6b773e15/base/metrics/sample_vector.h
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d1c910879ed03b3016a4f51ca1a1080534e8cbf2 commit d1c910879ed03b3016a4f51ca1a1080534e8cbf2 Author: Brian White <bcwhite@chromium.org> Date: Fri Nov 03 14:46:42 2017 Move string storage out of HistogramBase class. With the move to persistent histograms, a copy of the histogram name is saved in order to reconstruct the histogram at a later time. It is therefore unnecessary to have a std::string object inside each histogram when they can point directly to the string in the persistent memory. This saves 8-24 bytes on 32-bit builds and roughly double that on 64-bit builds. The range is due to different implementations of std::string, some of which include direct storage for small strings. This amounts to a substantial memory savings (>50% per object on some buids) given the 3000+ histogram objects maintained by a typical Chrome process. For non-persistent histograms, the strings will be copied to a separate memory area maintaining consistency with the existing interface that doesn't guarantee passing immutable strings. Bug: 733380 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I1467985b5755310b968174425eaf38db051bac2f Reviewed-on: https://chromium-review.googlesource.com/741343 Reviewed-by: Stephen Lanham <slan@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Dominic Battré <battre@chromium.org> Reviewed-by: Tommi <tommi@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Brian White <bcwhite@chromium.org> Cr-Commit-Position: refs/heads/master@{#513768} [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram.h [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram_base.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram_base.h [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram_base_unittest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/histogram_snapshot_manager.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/persistent_histogram_allocator.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/sparse_histogram.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/sparse_histogram.h [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/sparse_histogram_unittest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/statistics_recorder.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/base/metrics/statistics_recorder_unittest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/chrome/browser/extensions/api/metrics_private/metrics_apitest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/chromecast/base/metrics/grouped_histogram.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/components/metrics/file_metrics_provider_unittest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/components/prefs/json_pref_store_unittest.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/extensions/browser/value_store/lazy_leveldb.h [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/extensions/browser/value_store/leveldb_value_store.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/third_party/webrtc_overrides/init_webrtc.cc [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/tools/metrics/histograms/histograms.xml [modify] https://crrev.com/d1c910879ed03b3016a4f51ca1a1080534e8cbf2/ui/gl/angle_platform_impl.cc
,
Nov 3 2017
The final tally is... 32-bit builds: reduced by 72 to 88 bytes 64-bit builds: reduced by 96 to 112 bytes The range is because the std::string that was eliminated is 12/24 bytes on some builds and 28/40 bytes on others depending on whether it has a "small string optimization buffer" included. Multiply this by some 5000 instances across multiple processes and it saves roughly 500KiB of memory.
,
Nov 6 2017
That final CL also cuts in 1/2 the string storage (when persistence is enabled) so subtract around another 64*5000 ~= 320 KiB.
,
Nov 6 2017
That's really awesome! Thanks Brian! |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by asvitk...@chromium.org
, Jun 16 2017