Optimize histogram storage for memory |
||||
Issue descriptionMoving from the email discussion into a bug. There were two ideas identified as some ways we could save on metrics memory: 1) "Each histogram can have built-in space to store exactly one sample and only allocate the buckets array one there's a second sample. For example, all startup histograms only log the value once, but allocate space for all the buckets that is wasted regardless." 2) The "ranges" description is stored persistently for each histogram. I can de-dup those. Not sure the exact impact but I expect "significant" (meaning >10%). Brian, would you be able to take this on?
,
Apr 4 2017
Updating some labels. Brian started work on this already and has CL under review for the ranges optimization. From his local analysis: About 261 KiB [is saved] out of 1138 KiB used in the browser process after a fresh startup and loading three pages: chrome://histograms, https://www.google.com/, and again chrome://histograms. Normal usage is about 3x that; savings should scale linearly. (I'm assuming those are results from Windows - although percentage-wise the savings should be similar on other platforms.)
,
Apr 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af1913582fdf411b2f7edc84189c78b1fb402a37 commit af1913582fdf411b2f7edc84189c78b1fb402a37 Author: bcwhite <bcwhite@chromium.org> Date: Fri Apr 07 17:02:01 2017 De-duplicate ranges information in persistent memory. By having BucketRanges keep a reference to where the data can be found in persistent memory, the existing de-dup of the BucketRanges object also serves to avoid duplicating the data in persistent memory. Simple testing shows allocated persistent histogram memory in the browser is reduced by about 22%. This should be similar in all processes though somewhat less as the number of histograms held goes down. BUG= 705342 Review-Url: https://codereview.chromium.org/2794073002 Cr-Commit-Position: refs/heads/master@{#462897} [modify] https://crrev.com/af1913582fdf411b2f7edc84189c78b1fb402a37/base/metrics/bucket_ranges.h [modify] https://crrev.com/af1913582fdf411b2f7edc84189c78b1fb402a37/base/metrics/persistent_histogram_allocator.cc [modify] https://crrev.com/af1913582fdf411b2f7edc84189c78b1fb402a37/base/metrics/persistent_histogram_allocator_unittest.cc [modify] https://crrev.com/af1913582fdf411b2f7edc84189c78b1fb402a37/base/metrics/statistics_recorder.cc
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1166f8da78cfb4071f088e0f85fd940429d785fe commit 1166f8da78cfb4071f088e0f85fd940429d785fe Author: bcwhite <bcwhite@chromium.org> Date: Fri Apr 21 17:19:03 2017 Support delayed allocations from persistent memory. This allows for an allocation to be defined by code that knows about persistent allocation but not be realized until more generic code actually needs the space. In addition, delayed allocations can be split and shared such that once its needed in one place it will be available in all places, an all-or- nothing arrangement. This is done in support of follow-up CL: https://codereview.chromium.org/2811713003/ BUG= 705342 Review-Url: https://codereview.chromium.org/2806403002 Cr-Commit-Position: refs/heads/master@{#466374} [modify] https://crrev.com/1166f8da78cfb4071f088e0f85fd940429d785fe/base/metrics/persistent_memory_allocator.cc [modify] https://crrev.com/1166f8da78cfb4071f088e0f85fd940429d785fe/base/metrics/persistent_memory_allocator.h [modify] https://crrev.com/1166f8da78cfb4071f088e0f85fd940429d785fe/base/metrics/persistent_memory_allocator_unittest.cc
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4162bafec82ea11c98d4dc2572337a039b79fdb9 commit 4162bafec82ea11c98d4dc2572337a039b79fdb9 Author: bcwhite <bcwhite@chromium.org> Date: Fri Apr 21 18:58:00 2017 Embed a single sample in histogram metadata. This uses the new DelayedPersistentAllocation to avoid allocating the full "counts" array when only one value is being stored. That one value is kept in the HistogramSamples metadata. When a second value is recorded, the counts-array is created and the single-sample is moved; it is there-after unused. This change only affects SampleVector. A future change may add the same support for PersistentSampleMap. A quick test shows persistent memory use is reduced by 44% (from 924 KiB to 512 KiB) after a clean start and the loading of three pages: chrome://histograms https://www.google.com/ chrome://histograms (again) BUG= 705342 Review-Url: https://codereview.chromium.org/2811713003 Cr-Commit-Position: refs/heads/master@{#466401} [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/histogram.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/histogram.h [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/histogram_samples.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/histogram_samples.h [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/histogram_unittest.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/persistent_histogram_allocator.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/persistent_sample_map.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/sample_map.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/sample_vector.cc [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/sample_vector.h [modify] https://crrev.com/4162bafec82ea11c98d4dc2572337a039b79fdb9/base/metrics/sample_vector_unittest.cc
,
Apr 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94191025bc29f325d91565c9520af6495f22be09 commit 94191025bc29f325d91565c9520af6495f22be09 Author: tdanderson <tdanderson@chromium.org> Date: Fri Apr 21 23:19:50 2017 Revert of Embed a single sample in histogram metadata. (patchset #9 id:240001 of https://codereview.chromium.org/2811713003/ ) Reason for revert: Speculative revert for suspected cause of failing NonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug test on Mac10.9 Tests (dbg). See https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/39679 Original issue's description: > Embed a single sample in histogram metadata. > > This uses the new DelayedPersistentAllocation to avoid allocating the > full "counts" array when only one value is being stored. That one > value is kept in the HistogramSamples metadata. When a second value > is recorded, the counts-array is created and the single-sample is > moved; it is there-after unused. > > This change only affects SampleVector. A future change may add the > same support for PersistentSampleMap. > > A quick test shows persistent memory use is reduced by 44% (from 924 > KiB to 512 KiB) after a clean start and the loading of three pages: > chrome://histograms > https://www.google.com/ > chrome://histograms (again) > > BUG= 705342 > > Review-Url: https://codereview.chromium.org/2811713003 > Cr-Commit-Position: refs/heads/master@{#466401} > Committed: https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc2572337a039b79fdb9 TBR=asvitkine@chromium.org,bcwhite@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 705342 Review-Url: https://codereview.chromium.org/2832333002 Cr-Commit-Position: refs/heads/master@{#466488} [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/histogram.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/histogram.h [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/histogram_samples.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/histogram_samples.h [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/histogram_unittest.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/persistent_histogram_allocator.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/persistent_sample_map.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/sample_map.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/sample_vector.cc [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/sample_vector.h [modify] https://crrev.com/94191025bc29f325d91565c9520af6495f22be09/base/metrics/sample_vector_unittest.cc
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb commit fa8485bd1548ab76854d5d01ea2c4cd31d719aeb Author: bcwhite <bcwhite@chromium.org> Date: Mon May 01 16:43:25 2017 Embed a single sample in histogram metadata. This uses the new DelayedPersistentAllocation to avoid allocating the full "counts" array when only one value is being stored. That one value is kept in the HistogramSamples metadata. When a second value is recorded, the counts-array is created and the single-sample is moved; it is there-after unused. This change only affects SampleVector. A future change may add the same support for PersistentSampleMap. A quick test shows persistent memory use is reduced by 44% (from 924 KiB to 512 KiB) after a clean start and the loading of three pages: chrome://histograms https://www.google.com/ chrome://histograms (again) BUG= 705342 Review-Url: https://codereview.chromium.org/2811713003 Cr-Original-Commit-Position: refs/heads/master@{#466401} Committed: https://chromium.googlesource.com/chromium/src/+/4162bafec82ea11c98d4dc2572337a039b79fdb9 Review-Url: https://codereview.chromium.org/2811713003 Cr-Commit-Position: refs/heads/master@{#468325} [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/histogram.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/histogram.h [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/histogram_samples.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/histogram_samples.h [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/histogram_unittest.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/persistent_histogram_allocator.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/persistent_sample_map.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/sample_map.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/sample_vector.cc [modify] https://crrev.com/fa8485bd1548ab76854d5d01ea2c4cd31d719aeb/base/metrics/sample_vector.h
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da4b99838d459396d412bc278358b7f335ac0fdc commit da4b99838d459396d412bc278358b7f335ac0fdc Author: bcwhite <bcwhite@chromium.org> Date: Mon May 01 20:17:39 2017 Restore tests for single-sample storage in histograms. These tests were removed from the main CL because they were believed to be the cause of some instability in other tests. Without the tests, everything seems fine so now it's time to restore these. Main CL: https://codereview.chromium.org/2811713003/ BUG= 705342 Review-Url: https://codereview.chromium.org/2850233002 Cr-Commit-Position: refs/heads/master@{#468396} [modify] https://crrev.com/da4b99838d459396d412bc278358b7f335ac0fdc/base/metrics/sample_vector_unittest.cc
,
May 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/520284a7cdbcbd5fad7f0ee1bf3a79346e823999 commit 520284a7cdbcbd5fad7f0ee1bf3a79346e823999 Author: sky <sky@chromium.org> Date: Mon May 01 21:47:15 2017 Revert of Restore tests for single-sample storage in histograms. (patchset #1 id:1 of https://codereview.chromium.org/2850233002/ ) Reason for revert: The mac dbg builder has started failing again. See https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_Tests__dbg_%2F40054%2F%2B%2Frecipes%2Fsteps%2Fbase_unittests%2F0%2Flogs%2FNonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug%2F0 : [ RUN ] NonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug [WARNING] ../../testing/gtest/src/gtest-death-test.cc:834:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 4 threads. ../../base/threading/non_thread_safe_unittest.cc:140: Failure Death test: { NonThreadSafeClass::DestructorOnDifferentThreadImpl(); } Result: died but not with expected error. Expected: Check failed Actual msg: [ DEATH ] The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec(). [ DEATH ] Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug. [ DEATH ] The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec(). [ DEATH ] Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug. [ DEATH ] Received signal 11 SEGV_MAPERR 000000000110 [ DEATH ] [0x00010bd9c12e] [ DEATH ] [0x00010bd9c1cd] [ DEATH ] [0x00010bd9a65c] [ DEATH ] [0x00010bd9c017] [ DEATH ] [0x7fff915c85aa] [ DEATH ] [0x000000000000] [ DEATH ] [0x7fff95a203f9] [ DEATH ] [0x7fff95a2001b] [ DEATH ] [0x7fff95a1fbed] [ DEATH ] [0x7fff957349cb] [ DEATH ] [0x7fff90a97899] [ DEATH ] [0x7fff90a9772a] [ DEATH ] [0x7fff90a9bfc9] [ DEATH ] [end of stack trace] [ DEATH ] [ FAILED ] NonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug (533 ms) From looking at 705342 it seems as though your changes failed with a similar error before and were reverted. Specifically NonThreadSafeDeathTest.DestructorNotAllowedOnDifferentThreadInDebug . I'm going to revert and see if it fixes things. Original issue's description: > Restore tests for single-sample storage in histograms. > > These tests were removed from the main CL because they were > believed to be the cause of some instability in other tests. > Without the tests, everything seems fine so now it's time to > restore these. > > Main CL: https://codereview.chromium.org/2811713003/ > > BUG= 705342 > > Review-Url: https://codereview.chromium.org/2850233002 > Cr-Commit-Position: refs/heads/master@{#468396} > Committed: https://chromium.googlesource.com/chromium/src/+/da4b99838d459396d412bc278358b7f335ac0fdc TBR=asvitkine@chromium.org,bcwhite@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 705342 Review-Url: https://codereview.chromium.org/2851363002 Cr-Commit-Position: refs/heads/master@{#468430} [modify] https://crrev.com/520284a7cdbcbd5fad7f0ee1bf3a79346e823999/base/metrics/sample_vector_unittest.cc
,
May 2 2017
Issue 717404 has been merged into this issue.
,
May 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ddc891f2d023a185db9c5a3c4f52bd728e117ed commit 0ddc891f2d023a185db9c5a3c4f52bd728e117ed Author: bcwhite <bcwhite@chromium.org> Date: Mon May 08 16:04:21 2017 Restore tests for single-sample storage in histograms. These tests were removed from the main CL because they were believed to be the cause of some instability in other tests. Without the tests, everything seems fine so now it's time to restore these. Main CL: https://codereview.chromium.org/2811713003/ BUG= 705342 Review-Url: https://codereview.chromium.org/2850233002 Cr-Original-Commit-Position: refs/heads/master@{#468396} Committed: https://chromium.googlesource.com/chromium/src/+/da4b99838d459396d412bc278358b7f335ac0fdc Review-Url: https://codereview.chromium.org/2850233002 Cr-Commit-Position: refs/heads/master@{#469996} [modify] https://crrev.com/0ddc891f2d023a185db9c5a3c4f52bd728e117ed/base/metrics/sample_vector_unittest.cc
,
May 10 2017
How much more work is required before we can close this out?
,
May 10 2017
There's one more optimization which is to remove the duplication of ranges data between persistent and heap memory. Not sure what the impact of that will be; I guess less than 100 KB. Not sure when I'll get to it.
,
May 30 2017
This is considered "done" with the existing changes which cover the majority of the expected savings. There is one further possible optimization (merging the in-ram ranges with the persistent copy) but it's relatively small and complicated by concurrency issues.
,
Jul 27 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by bcwh...@chromium.org
, Mar 27 2017