Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856663002:
Don't report array buffer allocations less than 1 Mb. (Closed)
DescriptionDon't report array buffer allocations less than 1 Mb.
History has shown that 99.93% (or more) of all memory allocations are less
than 1 megabyte, and they all appear in the same UMA stat entry.
To give perspective, the entry for <= 1Mb is about 20,000 times larger
than any other entry in the table. This makes the distribution in the
table hard to see.
And, for allocation failures at this size, the percentage of failures
(when compared to number of requests) is soo small (millions to one)
that little data can be gleamed from the <= 1Mb entry.
Note: requires CL https://codereview.chromium.org/2867483002
BUG=chrome:704922
R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org
Review-Url: https://codereview.chromium.org/2856663002
Cr-Commit-Position: refs/heads/master@{#45148}
Committed: https://chromium.googlesource.com/v8/v8/+/3d0535065d83420c4303fcff277b97d2ebfa0e35
Patch Set 1 #
Total comments: 4
Patch Set 2 : Apply s/Big/Large/ in V8.ArrayBufferBigAllocations. #
Messages
Total messages: 27 (16 generated)
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. BUG=chrome:704922 ========== to ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. BUG=chrome:704922 R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org ==========
kschimpf@chromium.org changed reviewers: + bbudge@chromium.org, bradnelson@chromium.org, isherman@chromium.org
https://codereview.chromium.org/2856663002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2856663002/diff/1/src/counters.h#newcode938 src/counters.h:938: HR(array_buffer_big_allocations, V8.ArrayBufferBigAllocations, 1, 4096, 13) \ Please rename the histogram, as you are changing the bucket layout for it. Otherwise, you'll get data mixed in from multiple versions, and the blended data will look rather confusing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2856663002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2856663002/diff/1/src/counters.h#newcode938 src/counters.h:938: HR(array_buffer_big_allocations, V8.ArrayBufferBigAllocations, 1, 4096, 13) \ On 2017/05/01 19:51:14, Ilya Sherman wrote: > Please rename the histogram, as you are changing the bucket layout for it. > Otherwise, you'll get data mixed in from multiple versions, and the blended data > will look rather confusing. Is this really necessary. This metric was created very recently and isn't really being used anywhere (yet). Otherwise, I'd be more willing to change this back to starting at zero.
https://codereview.chromium.org/2856663002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2856663002/diff/1/src/counters.h#newcode938 src/counters.h:938: HR(array_buffer_big_allocations, V8.ArrayBufferBigAllocations, 1, 4096, 13) \ On 2017/05/04 20:51:36, kschimpf wrote: > On 2017/05/01 19:51:14, Ilya Sherman wrote: > > Please rename the histogram, as you are changing the bucket layout for it. > > Otherwise, you'll get data mixed in from multiple versions, and the blended > data > > will look rather confusing. > > Is this really necessary. This metric was created very recently and isn't really > being used anywhere (yet). > > Otherwise, I'd be more willing to change this back to starting at zero. Well, I think it might actually be the same bucket layout regardless of whether the first value is 0 or 1 -- I think the code will automatically correct a 0 to a 1, and reserve the zero-th bucket as an underflow bucket. I'd strongly encourage you to double-check me on this, as I'm not familiar with what exactly these V8 wrappers are doing under the hood. If the actual bucket layout is changing, then you should definitely either (a) restore the previous layout, or (b) rename the histogram. However, you're still blending data with rather different distributions across multiple versions. The best practice would be to rename the histogram. The next-best option would be to document the change in semantics in the histograms.xml description... though such documentation is very easy for readers to miss. What is your concern w.r.t. renaming the histogram?
Description was changed from ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. BUG=chrome:704922 R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org ========== to ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. Note: requires CL https://codereview.chromium.org/2867483002 BUG=chrome:704922 R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org ==========
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2856663002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2856663002/diff/1/src/counters.h#newcode938 src/counters.h:938: HR(array_buffer_big_allocations, V8.ArrayBufferBigAllocations, 1, 4096, 13) \ On 2017/05/04 21:21:21, Ilya Sherman wrote: > On 2017/05/04 20:51:36, kschimpf wrote: > > On 2017/05/01 19:51:14, Ilya Sherman wrote: > > > Please rename the histogram, as you are changing the bucket layout for it. > > > Otherwise, you'll get data mixed in from multiple versions, and the blended > > data > > > will look rather confusing. > > > > Is this really necessary. This metric was created very recently and isn't > really > > being used anywhere (yet). > > > > Otherwise, I'd be more willing to change this back to starting at zero. > > Well, I think it might actually be the same bucket layout regardless of whether > the first value is 0 or 1 -- I think the code will automatically correct a 0 to > a 1, and reserve the zero-th bucket as an underflow bucket. I'd strongly > encourage you to double-check me on this, as I'm not familiar with what exactly > these V8 wrappers are doing under the hood. If the actual bucket layout is > changing, then you should definitely either (a) restore the previous layout, or > (b) rename the histogram. > > However, you're still blending data with rather different distributions across > multiple versions. The best practice would be to rename the histogram. The > next-best option would be to document the change in semantics in the > histograms.xml description... though such documentation is very easy for readers > to miss. > > What is your concern w.r.t. renaming the histogram? Ok. Changing the name of the metric, and introducing it into V8.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM, thanks.
The CQ bit was checked by kschimpf@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/25477)
The CQ bit was checked by kschimpf@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494097742046590,
"parent_rev": "47a8e354c7248e2e59705c1907cfb30b2223c876", "commit_rev":
"3d0535065d83420c4303fcff277b97d2ebfa0e35"}
Message was sent while issue was closed.
Description was changed from ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. Note: requires CL https://codereview.chromium.org/2867483002 BUG=chrome:704922 R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org ========== to ========== Don't report array buffer allocations less than 1 Mb. History has shown that 99.93% (or more) of all memory allocations are less than 1 megabyte, and they all appear in the same UMA stat entry. To give perspective, the entry for <= 1Mb is about 20,000 times larger than any other entry in the table. This makes the distribution in the table hard to see. And, for allocation failures at this size, the percentage of failures (when compared to number of requests) is soo small (millions to one) that little data can be gleamed from the <= 1Mb entry. Note: requires CL https://codereview.chromium.org/2867483002 BUG=chrome:704922 R=bradnelson@chromium.org, bbudge@chromium.org, isherman@chromium.org Review-Url: https://codereview.chromium.org/2856663002 Cr-Commit-Position: refs/heads/master@{#45148} Committed: https://chromium.googlesource.com/v8/v8/+/3d0535065d83420c4303fcff277b97d2ebf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/3d0535065d83420c4303fcff277b97d2ebf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
