Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
984803002:
content: Add histogram for discardable memory allocation size. (Closed)
|
Created:
5 years, 9 months ago by reveman Modified:
5 years, 9 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, Daniele Castagna, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent: Add histogram for discardable memory allocation size.
The discardable memory free list might need to be split into
different buckets for good performance. These histograms will
be useful when choosing bucket sizes.
BUG=
Committed: https://crrev.com/e15f9683681355901967b8d0f8e23715b6098ff0
Cr-Commit-Position: refs/heads/master@{#319654}
Patch Set 1 #
Total comments: 5
Patch Set 2 : #
Total comments: 4
Patch Set 3 : add "in KB" to description #
Messages
Total messages: 16 (3 generated)
reveman@chromium.org changed reviewers: + asvitkine@chromium.org, avi@chromium.org
lgtm
lgtm https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... content/child/child_discardable_shared_memory_manager.cc:72: 0, There is already a default overflow bucket between 0 and the min value you specify here. So this should be 1. https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... content/child/child_discardable_shared_memory_manager.cc:74: 256); Why do you need 256 buckets? This is much more than other histograms and I'm skeptical you actual need this many, but happy to discuss the details.
oops, not lgtm - sorry clicked wrong button
PTAL https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... content/child/child_discardable_shared_memory_manager.cc:72: 0, On 2015/03/06 at 15:49:08, Alexei Svitkine wrote: > There is already a default overflow bucket between 0 and the min value you specify here. So this should be 1. Done. https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... content/child/child_discardable_shared_memory_manager.cc:74: 256); On 2015/03/06 at 15:49:08, Alexei Svitkine wrote: > Why do you need 256 buckets? This is much more than other histograms and I'm skeptical you actual need this many, but happy to discuss the details. Reduced it to 50 which is the same as UMA_HISTOGRAM_MEMORY_KB. I'm not sure that's enough but we can start with that. The majority of samples will be in the 1-1000KB range with some samples anywhere from 1MB to multiple GBs. I'm interested in both the distribution of samples in the 0-1MB range as well as the more rare large allocations. Maybe I can use a logarithmic scale..
https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/984803002/diff/1/content/child/child_discarda... content/child/child_discardable_shared_memory_manager.cc:74: 256); On 2015/03/07 23:05:45, reveman wrote: > On 2015/03/06 at 15:49:08, Alexei Svitkine wrote: > > Why do you need 256 buckets? This is much more than other histograms and I'm > skeptical you actual need this many, but happy to discuss the details. > > Reduced it to 50 which is the same as UMA_HISTOGRAM_MEMORY_KB. I'm not sure > that's enough but we can start with that. The majority of samples will be in the > 1-1000KB range with some samples anywhere from 1MB to multiple GBs. I'm > interested in both the distribution of samples in the 0-1MB range as well as the > more rare large allocations. Maybe I can use a logarithmic scale.. Just realized that the bucket layout is already exponential by default so 50 buckets as in the latest patch should be more than enough. Sorry for the noise..
lgtm https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14069: +<histogram name="Memory.DiscardableAllocationSize" units="KB"> Nit: Maybe name it Memory.DiscardableAllocationSizeKB
https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14069: +<histogram name="Memory.DiscardableAllocationSize" units="KB"> On 2015/03/09 14:15:44, Alexei Svitkine wrote: > Nit: Maybe name it Memory.DiscardableAllocationSizeKB There doesn't seem to be any general consistency in regards to including the unit in the name or not. I would prefer not to include it to be consistent with other Memory.* histograms and it seems a bit superfluous to have it in the name when we have units="KB". Or am I missing some good reason for having it in the name?
lgtm https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14069: +<histogram name="Memory.DiscardableAllocationSize" units="KB"> On 2015/03/09 15:12:40, reveman wrote: > On 2015/03/09 14:15:44, Alexei Svitkine wrote: > > Nit: Maybe name it Memory.DiscardableAllocationSizeKB > > There doesn't seem to be any general consistency in regards to including the > unit in the name or not. I would prefer not to include it to be consistent with > other Memory.* histograms and it seems a bit superfluous to have it in the name > when we have units="KB". Or am I missing some good reason for having it in the > name? I think not all our dashboards show the units text. If you prefer to leave out of the name, I would at least mention it in the description. (i.e. "allocation size in KB.")
https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/984803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14069: +<histogram name="Memory.DiscardableAllocationSize" units="KB"> On 2015/03/09 15:17:13, Alexei Svitkine wrote: > On 2015/03/09 15:12:40, reveman wrote: > > On 2015/03/09 14:15:44, Alexei Svitkine wrote: > > > Nit: Maybe name it Memory.DiscardableAllocationSizeKB > > > > There doesn't seem to be any general consistency in regards to including the > > unit in the name or not. I would prefer not to include it to be consistent > with > > other Memory.* histograms and it seems a bit superfluous to have it in the > name > > when we have units="KB". Or am I missing some good reason for having it in the > > name? > > I think not all our dashboards show the units text. If you prefer to leave out > of the name, I would at least mention it in the description. (i.e. "allocation > size in KB.") Ok, thanks. Done.
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/984803002/#ps40001 (title: "add "in KB" to description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/984803002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e15f9683681355901967b8d0f8e23715b6098ff0 Cr-Commit-Position: refs/heads/master@{#319654} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
