Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795763002:
Collect array buffer allocation sizes for UMA in megabytes. (Closed)
DescriptionCollect array buffer allocation sizes for UMA in megabytes.
After discussion with Chrome reviewers for UMA, it was decided that we
would report array buffer allocation sizes in megabytes (not the log).
They also wanted to wait until there is proof that small array buffer
allocations would flood the histogram. Hence, all allocation sizes are
sampled.
There were several ways we could have added the notion of megabyte
samples to V8 code. None of them are a great fit. This code simply
provides a local function within the code that needs it.
Other possible solutions but rejected were:
a) Use a subclass of histogram to collect data at the megabyte level.
It has it's own Add() method that converts the size from bytes to
megabytes, and then call the generic add method AddSample(). This
solution appears to follow the conventions of subclasses of class
Histogram.
b) Use Chrome macros - Rejected because it involves changing the
counter representation of V8.
c) Add a method AddMegabyteSample() to base class Histogram. Rejected
because it may get confusing if a lot of different measures are
added the the base class of histograms.
d) Make method AddSample() virtual and override in the derived
class. Rejected in that sampling is supposed to be fast, and adding
a virtual call may be breaking that contract.
d) Do not add a derived class. Rather just do the conversions at the
call sites. Rejected because this duplicates code, and also makes
it hard to change assumptions on how to calculate.
For Chromes UMA changes see:
CL: https://codereview.chromium.org/2795463002
BUG=chromium:704922
R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org
Review-Url: https://codereview.chromium.org/2795763002
Cr-Commit-Position: refs/heads/master@{#44388}
Committed: https://chromium.googlesource.com/v8/v8/+/510abe732e132b8a876374b13d5f632086064a72
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix call to AddSample to cast argument to int. #Patch Set 3 : Fix that allocation size can be zero. #
Total comments: 2
Patch Set 4 : Localize size conversion. #
Total comments: 2
Patch Set 5 : Fix name of convertToMG. #Messages
Total messages: 28 (19 generated)
The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run
Please review. Thanks!
Dry run: 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
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
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...
lgtm, 1 nit. https://codereview.chromium.org/2795763002/diff/1/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2795763002/diff/1/src/counters.cc#newcode85 src/counters.cc:85: const char* caption; could we initialize the members in-place? (caption = nullptr.. etc)
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/2795763002/diff/1/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2795763002/diff/1/src/counters.cc#newcode85 src/counters.cc:85: const char* caption; On 2017/04/03 17:26:20, Mircea Trofin wrote: > could we initialize the members in-place? (caption = nullptr.. etc) Explicit initialization is done in the entries using macro MEGABYTE_HISTOGRAM_RANGE_LIST.
https://codereview.chromium.org/2795763002/diff/40001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2795763002/diff/40001/src/counters.h#newcode244 src/counters.h:244: }; This seems like a lot just to scale the sizes, and it might be confusing that there's an AddSample method on the class. Rather than inherit a new class, why not create a static conversion method? i.e. int SizeInMegabytes(size_t size) { ...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Collect array buffer allocation sizes for UMA in megabytes. After discussion with Chrome reviewers for UMA, it was decided that we would report array buffer allocation sizes in megabytes (not the log). They also wanted to wait until there is proof that small array buffer allocations would flood the histogram. Hence, all allocation sizes are sampled. There were several ways we could have added the notion of megabyte samples to V8 code. None of them are a great fit. This code uses a subclass of histogram to collect data at the megabyte level. It has it's own Add() method that converts the size from bytes to megabytes, and then call the generic add method AddSample(). This solution appears to follow the conventions of subclasses of class Histogram. Other possible solutions but rejected were: a) Use Chrome macros - Rejected because it involves changing the counter representation of V8. b) Add a method AddMegabyteSample() to base class Histogram. Rejected because it may get confusing if a lot of different measures are added the the base class of histograms. c) Make method AddSample() virtual and override in the derived class. Rejected in that sampling is supposed to be fast, and adding a virtual call may be breaking that contract. d) Do not add a derived class. Rather just do the conversions at the call sites. Rejected because this duplicates code, and also makes it hard to change assumptions on how to calculate. For Chromes UMA changes see: CL: https://codereview.chromium.org/2795463002 BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ========== to ========== Collect array buffer allocation sizes for UMA in megabytes. After discussion with Chrome reviewers for UMA, it was decided that we would report array buffer allocation sizes in megabytes (not the log). They also wanted to wait until there is proof that small array buffer allocations would flood the histogram. Hence, all allocation sizes are sampled. There were several ways we could have added the notion of megabyte samples to V8 code. None of them are a great fit. This code simply provides a local function within the code that needs it. Other possible solutions but rejected were: a) Use a subclass of histogram to collect data at the megabyte level. It has it's own Add() method that converts the size from bytes to megabytes, and then call the generic add method AddSample(). This solution appears to follow the conventions of subclasses of class Histogram. b) Use Chrome macros - Rejected because it involves changing the counter representation of V8. c) Add a method AddMegabyteSample() to base class Histogram. Rejected because it may get confusing if a lot of different measures are added the the base class of histograms. d) Make method AddSample() virtual and override in the derived class. Rejected in that sampling is supposed to be fast, and adding a virtual call may be breaking that contract. d) Do not add a derived class. Rather just do the conversions at the call sites. Rejected because this duplicates code, and also makes it hard to change assumptions on how to calculate. For Chromes UMA changes see: CL: https://codereview.chromium.org/2795463002 BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@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/2795763002/diff/40001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2795763002/diff/40001/src/counters.h#newcode244 src/counters.h:244: }; On 2017/04/03 18:02:41, bbudge wrote: > This seems like a lot just to scale the sizes, and it might be confusing that > there's an AddSample method on the class. Rather than inherit a new class, why > not create a static conversion method? > > i.e. > > int SizeInMegabytes(size_t size) { ... > After considerable discussion, we decided to move this locally to object.cc for the moment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/nit https://codereview.chromium.org/2795763002/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2795763002/diff/60001/src/objects.cc#newcode1... src/objects.cc:19468: inline int convertToMb(size_t size) { nit: ConvertToMb (first letter upper case for free function.)
The CQ bit was checked by kschimpf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org, bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/2795763002/#ps80001 (title: "Fix name of convertToMG.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2795763002/diff/60001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/2795763002/diff/60001/src/objects.cc#newcode1... src/objects.cc:19468: inline int convertToMb(size_t size) { On 2017/04/03 23:11:40, bbudge wrote: > nit: ConvertToMb (first letter upper case for free function.) Done.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491325190268090,
"parent_rev": "0bd9f1b8e60466fcfbbb01b5d048c72e3d01d361", "commit_rev":
"510abe732e132b8a876374b13d5f632086064a72"}
Message was sent while issue was closed.
Description was changed from ========== Collect array buffer allocation sizes for UMA in megabytes. After discussion with Chrome reviewers for UMA, it was decided that we would report array buffer allocation sizes in megabytes (not the log). They also wanted to wait until there is proof that small array buffer allocations would flood the histogram. Hence, all allocation sizes are sampled. There were several ways we could have added the notion of megabyte samples to V8 code. None of them are a great fit. This code simply provides a local function within the code that needs it. Other possible solutions but rejected were: a) Use a subclass of histogram to collect data at the megabyte level. It has it's own Add() method that converts the size from bytes to megabytes, and then call the generic add method AddSample(). This solution appears to follow the conventions of subclasses of class Histogram. b) Use Chrome macros - Rejected because it involves changing the counter representation of V8. c) Add a method AddMegabyteSample() to base class Histogram. Rejected because it may get confusing if a lot of different measures are added the the base class of histograms. d) Make method AddSample() virtual and override in the derived class. Rejected in that sampling is supposed to be fast, and adding a virtual call may be breaking that contract. d) Do not add a derived class. Rather just do the conversions at the call sites. Rejected because this duplicates code, and also makes it hard to change assumptions on how to calculate. For Chromes UMA changes see: CL: https://codereview.chromium.org/2795463002 BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org ========== to ========== Collect array buffer allocation sizes for UMA in megabytes. After discussion with Chrome reviewers for UMA, it was decided that we would report array buffer allocation sizes in megabytes (not the log). They also wanted to wait until there is proof that small array buffer allocations would flood the histogram. Hence, all allocation sizes are sampled. There were several ways we could have added the notion of megabyte samples to V8 code. None of them are a great fit. This code simply provides a local function within the code that needs it. Other possible solutions but rejected were: a) Use a subclass of histogram to collect data at the megabyte level. It has it's own Add() method that converts the size from bytes to megabytes, and then call the generic add method AddSample(). This solution appears to follow the conventions of subclasses of class Histogram. b) Use Chrome macros - Rejected because it involves changing the counter representation of V8. c) Add a method AddMegabyteSample() to base class Histogram. Rejected because it may get confusing if a lot of different measures are added the the base class of histograms. d) Make method AddSample() virtual and override in the derived class. Rejected in that sampling is supposed to be fast, and adding a virtual call may be breaking that contract. d) Do not add a derived class. Rather just do the conversions at the call sites. Rejected because this duplicates code, and also makes it hard to change assumptions on how to calculate. For Chromes UMA changes see: CL: https://codereview.chromium.org/2795463002 BUG=chromium:704922 R=bbudge@chromium.org,bradnelson@chromium.org,mtrofin@chromium.org Review-Url: https://codereview.chromium.org/2795763002 Cr-Commit-Position: refs/heads/master@{#44388} Committed: https://chromium.googlesource.com/v8/v8/+/510abe732e132b8a876374b13d5f6320860... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/510abe732e132b8a876374b13d5f6320860... |
