Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321823002:
histograms.xml: add five zram (compressed memory) histograms (Closed)
|
Created:
6 years, 6 months ago by Luigi Semenzato Modified:
6 years, 6 months ago CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@var-run Visibility:
Public. |
Descriptionhistograms.xml: add five zram (compressed memory) histograms
These histograms partially overlap with some already present
in Chrome, but there are a few differences, and the histograms
belong to Chrome OS metrics rather than Chrome metrics.
BUG=chromium:315113
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276916
Patch Set 1 #
Total comments: 2
Patch Set 2 : histograms.xml: add five zram (compressed memory) histograms #
Messages
Total messages: 10 (0 generated)
Hi Jim, would you be so kind to review this? Thanks!
On 2014/06/09 17:39:16, semenzato wrote: > Hi Jim, would you be so kind to review this? Thanks! Maybe Ilya would be so kind to review this? Thanks!
https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18472: + Compression ratio in percents (between 100% and 600%, i.e. a ratio between 1 Hmm, why is 600% the upper bound, and why is 100% the lower bound? IMO your metric would be clearer if it were typically a number between 0% and 100%, i.e. if you reversed the numerator and denominator. Keep in mind also that compression can, in principle, actually be *worse* than no compression, i.e. the compressed size can be greater than the uncompressed size.
On 2014/06/11 20:30:47, Ilya Sherman wrote: > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > tools/metrics/histograms/histograms.xml:18472: + Compression ratio in > percents (between 100% and 600%, i.e. a ratio between 1 > Hmm, why is 600% the upper bound, and why is 100% the lower bound? IMO your > metric would be clearer if it were typically a number between 0% and 100%, i.e. > if you reversed the numerator and denominator. Keep in mind also that > compression can, in principle, actually be *worse* than no compression, i.e. the > compressed size can be greater than the uncompressed size. There is a problem with the way we do these reviews. The code that sends these metrics has already been checked into a Chrome OS repo, so the actual quantities have already been discussed and approved by someone, and although your suggestions have merit, I would prefer not to go back and change that code. LZO compression in a worst-case situation results in a 0.4% increase, so 100% is good enough (however you look at it). Also, in practice we see ratios between 2.5 and 3 (250% and 300%) so anything much higher than 3 is suspicious as it would indicate we're compressing a lot of zero-filled pages, and there is a separate histogram to confirm that. Traditionally compression ratios are viewed as uncompressed size / compressed size, so a "better" compression ratio is a higher number. Ultimately I don't care, but that's the convention.
On 2014/06/11 21:20:07, semenzato wrote: > On 2014/06/11 20:30:47, Ilya Sherman wrote: > > > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > > tools/metrics/histograms/histograms.xml:18472: + Compression ratio in > > percents (between 100% and 600%, i.e. a ratio between 1 > > Hmm, why is 600% the upper bound, and why is 100% the lower bound? IMO your > > metric would be clearer if it were typically a number between 0% and 100%, > i.e. > > if you reversed the numerator and denominator. Keep in mind also that > > compression can, in principle, actually be *worse* than no compression, i.e. > the > > compressed size can be greater than the uncompressed size. > > There is a problem with the way we do these reviews. The code that sends these > metrics has already been checked into a Chrome OS repo, so the actual quantities > have already been discussed and approved by someone, and although your > suggestions have merit, I would prefer not to go back and change that code. I agree that it would be best to have a histograms.xml file that lives in the ChromeOS repository. If you have the energy for it, I'd encourage you to work with the UMA team to set that up. In the meantime, I'd suggest uploading both reviews at the same time, and waiting for both to be LGTM'ed prior to committing either side. I realize that that's a bit of extra hassle for you, but this way you can actually act on all review comments. This of course assumes that you find histograms.xml review comments worth acting on. > LZO compression in a worst-case situation results in a 0.4% increase, so 100% is > good enough (however you look at it). Also, in practice we see ratios between > 2.5 and 3 (250% and 300%) so anything much higher than 3 is suspicious as it > would indicate we're compressing a lot of zero-filled pages, and there is a > separate histogram to confirm that. > > Traditionally compression ratios are viewed as uncompressed size / compressed > size, so a "better" compression ratio is a higher number. Ultimately I don't > care, but that's the convention. Hmm, interesting -- I was unfamiliar with this convention. I find that a somewhat odd way to represent the data, but since it's a common convention, I find it a lot more palatable. Thanks for teaching me something :)
LGTM with a suggestion: https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:18476: + effectiveness. Snapshot every 30s. nit: Suggested phrasing tweaks, that help at least me to more quickly understand the prose. Feel free to further refine. "Compression ratio, expressed as a percentage (typically between 100% and 600%, i.e. a ratio between 1 and 6), when the compressed memory is at least 1 MB after compression. This is the ratio of the RAM used before compression (including zero pages) to the RAM used after compression. Values close to 100% indicate low compression effectiveness. Snapshot every 30s."
On 2014/06/11 21:37:30, Ilya Sherman wrote: > LGTM with a suggestion: > > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/321823002/diff/1/tools/metrics/histograms/his... > tools/metrics/histograms/histograms.xml:18476: + effectiveness. Snapshot > every 30s. > nit: Suggested phrasing tweaks, that help at least me to more quickly understand > the prose. Feel free to further refine. > > "Compression ratio, expressed as a percentage (typically between 100% and 600%, > i.e. a ratio between 1 and 6), when the compressed memory is at least 1 MB after > compression. This is the ratio of the RAM used before compression (including > zero pages) to the RAM used after compression. Values close to 100% indicate > low compression effectiveness. Snapshot every 30s." Thank you Ilya.
The CQ bit was checked by semenzato@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/semenzato@chromium.org/321823002/20001
Message was sent while issue was closed.
Change committed as 276916 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
