Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
362803002:
Track UMA stats for running out of tile memory (Closed)
DescriptionWe should add a histogram to track the frequency of running
out of memory for tiles in the memory budget.
BUG=368392
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282987
Patch Set 1 #Patch Set 2 : Changed the stats from memory exceeded bytes to OOM enabled. #
Total comments: 4
Patch Set 3 : Correct description of boolean UMA tracker for tile memory #
Total comments: 1
Patch Set 4 : Need to track oom at right place. #
Total comments: 4
Patch Set 5 : Changed histogram.xml as per comments. #
Total comments: 4
Patch Set 6 : Corrected histogram as per comments #
Messages
Total messages: 17 (0 generated)
PTAL..
I don't think we can keep this in the future without incurring a performance penalty and some awkward code. With the direction we're going, knowing the exact amount of memory needed is hard. We'll know when we're out of memory but not how much more is needed. Can you make this stat a boolean instead?
+ isherman for tools/metrics/histograms/histograms.xml changes. Ptal..
https://codereview.chromium.org/362803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31908: + Excess memory budget when tiles hard limited is crossed and oom is Look like this needs to be updated since it's a bool now.
https://codereview.chromium.org/362803002/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/362803002/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:878: UMA_HISTOGRAM_BOOLEAN("TileManager.ExceededMemoryBudget", true); I strongly encourage you to build in a baseline for this metric. Not all Chrome users report UMA data, and not all UMA users use Chrome equally. Suppose you see that in a day, 10,000 users exceeded the memory budget. What does that tell you? I bet that you'd deduce the answer by comparing 10,000 to a baseline that is currently implicit. Rather than having that baseline be implicit, which requires every consumer of this metric to correctly identify and compute the implicit baseline, you'd be better off building in an explicit baseline.
https://codereview.chromium.org/362803002/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/362803002/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:878: UMA_HISTOGRAM_BOOLEAN("TileManager.ExceededMemoryBudget", true); On 2014/07/02 21:08:37, Ilya Sherman wrote: > I strongly encourage you to build in a baseline for this metric. Not all Chrome > users report UMA data, and not all UMA users use Chrome equally. Suppose you > see that in a day, 10,000 users exceeded the memory budget. What does that tell > you? I bet that you'd deduce the answer by comparing 10,000 to a baseline that > is currently implicit. Rather than having that baseline be implicit, which > requires every consumer of this metric to correctly identify and compute the > implicit baseline, you'd be better off building in an explicit baseline. Thanks for the input. Do we want to add the custom counts here, is that what you mean by baseline? https://codereview.chromium.org/362803002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31908: + Excess memory budget when tiles hard limited is crossed and oom is On 2014/07/02 18:44:09, danakj wrote: > Look like this needs to be updated since it's a bool now. Done.
https://codereview.chromium.org/362803002/diff/40001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/362803002/diff/40001/cc/resources/tile_manage... cc/resources/tile_manager.cc:878: UMA_HISTOGRAM_BOOLEAN("TileManager.ExceededMemoryBudget", true); I think what we should have is UMA_HISTOGRAM_BOOLEAN(..., oomed_hard); The problem with the current approach is that we will never see a "false" in this boolean, so the usefulness of seeing "true" a bunch of times is questionable. Furthermore, if you report it in "ever exceeded" block, then once we exceed the memory then for the lifetime of tile manager, we will always report true (even if it goes back under the memory limit).
ptal.. as isherman suggested to use explicit baseline, does this mean the custom_count for boolean uma?
On 2014/07/10 15:13:49, Sikugu wrote: > ptal.. > > as isherman suggested to use explicit baseline, does this mean the custom_count > for boolean uma? I'm happy with this approach, but I'll leave the final review to reveman@ and isherman@
Thanks, this is indeed what I meant by including a baseline :) https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32132: +<histogram name="TileManager.ExceededMemoryBudget" enum="BooleanEnabled"> Please define a custom boolean enum with labels that precisely match the metric that you're measuring. https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32136: + Tracks if tile memory hard limited is crossed and oom is encountered. Please document when this metric is measured.
ptal.. https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32132: +<histogram name="TileManager.ExceededMemoryBudget" enum="BooleanEnabled"> On 2014/07/11 01:17:22, Ilya Sherman wrote: > Please define a custom boolean enum with labels that precisely match the metric > that you're measuring. Done. https://codereview.chromium.org/362803002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32136: + Tracks if tile memory hard limited is crossed and oom is encountered. On 2014/07/11 01:17:22, Ilya Sherman wrote: > Please document when this metric is measured. Done.
LGTM with the remaining comments addressed. Thanks. https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32139: + tilemanager. I meant that you should describe the condition that triggers logging of this metric, regardless of which bucket is emitted to. Here's my suggested rewording; feel free to further refine it: """ Measures whether the tile manager exceeded the hard GPU memory budget (OOMed). Recorded each time the tile manager assigns GPU memory to tiles. """ https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47194: + <int value="0" label="With in the memory budget"/> nit: "With in the" -> "Within"
Thanks Ilya Sherman. Reveman ptal.. https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32139: + tilemanager. On 2014/07/11 22:40:25, Ilya Sherman wrote: > I meant that you should describe the condition that triggers logging of this > metric, regardless of which bucket is emitted to. Here's my suggested > rewording; feel free to further refine it: > > """ > Measures whether the tile manager exceeded the hard GPU memory budget (OOMed). > Recorded each time the tile manager assigns GPU memory to tiles. > """ Done. https://codereview.chromium.org/362803002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:47194: + <int value="0" label="With in the memory budget"/> On 2014/07/11 22:40:25, Ilya Sherman wrote: > nit: "With in the" -> "Within" Done.
lgtm
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/362803002/100001
Message was sent while issue was closed.
Change committed as 282987 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
