Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 6 months ago by kschimpf Modified:
3 years, 6 months ago Reviewers:
Clemens Hammacher, Mircea Trofin, jochen (gone - plz use gerrit), Camillo Bruni CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionLocalize counter class member functions.
This CL takes advantage of the fact that StatsCounter is now local to
the Counters class. This includes:
1) Method StatsTable::SetCreateHistogramFunction() was only called in
one spot (in api.cc), which also called Counters::ResetHistograms()
and Counters::InitializeHistorgram(). InitializeHistogram can be
folded into Histogram.Reset().
2) Since Histogram::Reset() now regenerats the histogram, we no longer
need the field lookup_done_. Therefore there is no longer a race
between updating ptr_ and lookup_done_, making the Histogram class
thread safe.
3) Made the constructors of several classes private (except for class
Counters), minimizing the scope that they are used. When the couldn't
be moved, add comment that they were public only for test cases.
4) Removed the need for a mutex lock on StatsCounter::Reset(), since
it is now guaranteed to only be called when
StatsTable::SetCounterFunction() is called.
BUG=v8:6361
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
Review-Url: https://codereview.chromium.org/2918703002
Cr-Commit-Position: refs/heads/master@{#45791}
Committed: https://chromium.googlesource.com/v8/v8/+/f073a20b6999573a415a3f56912c812d1d737cbe
Patch Set 1 #Patch Set 2 : Initial patch. #Patch Set 3 : Fix nits in comments. #Patch Set 4 : Remove need for mutex to initialize counters. #
Total comments: 4
Patch Set 5 : Refactor so that isolates only communicate via the Counters class. #Patch Set 6 : Remove unnecessary outline definition. #Patch Set 7 : Fix runtime bug. #Patch Set 8 : Make constructors public and move friend declarations. #Patch Set 9 : Move ctors back to private. #
Created: 3 years, 6 months ago
Messages
Total messages: 33 (23 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 ========== to ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ==========
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 ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ========== to ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ==========
kschimpf@chromium.org changed reviewers: + cbruni@chromium.org, clemensh@chromium.org, jochen@chromium.org, mtrofin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please review. Thanks!
https://codereview.chromium.org/2918703002/diff/60001/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2918703002/diff/60001/src/counters.cc#newcode29 src/counters.cc:29: void StatsTable::SetCreateHistogramFunction(CreateHistogramCallback f) { It's a bit surprising setting the callback (which sounds like an accessor) has a reset as a side-effect. Would it be possible to not have a SetCreateHistogramFunction, and instead have it passed to the constructor? Alternatively, I'd add the "AndResetHistograms" to the name - yes, it'd be long; but that'd be the only shortcoming, and it'd reduce maintenance cost (one less surprise) https://codereview.chromium.org/2918703002/diff/60001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2918703002/diff/60001/src/counters.h#newcode200 src/counters.h:200: int* GetPtr(); Why not delete GetPtr(), since it's only used in Reset()?
https://codereview.chromium.org/2918703002/diff/60001/src/counters.cc File src/counters.cc (right): https://codereview.chromium.org/2918703002/diff/60001/src/counters.cc#newcode29 src/counters.cc:29: void StatsTable::SetCreateHistogramFunction(CreateHistogramCallback f) { On 2017/06/05 15:57:49, Mircea Trofin wrote: > It's a bit surprising setting the callback (which sounds like an accessor) has a > reset as a side-effect. Would it be possible to not have a > SetCreateHistogramFunction, and instead have it passed to the constructor? > > Alternatively, I'd add the "AndResetHistograms" to the name - yes, it'd be long; > but that'd be the only shortcoming, and it'd reduce maintenance cost (one less > surprise) Ok. However, the current name matches the "include/v8.h" API for class "Isolate" (which then did the same thing). I was really trying to move that logic into this counters.{h,cc} files to localize the internal implementation of the API. Changed the comments for these functions instead to intentionally not specify implementation details. https://codereview.chromium.org/2918703002/diff/60001/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2918703002/diff/60001/src/counters.h#newcode200 src/counters.h:200: int* GetPtr(); On 2017/06/05 15:57:49, Mircea Trofin wrote: > Why not delete GetPtr(), since it's only used in Reset()? Your right. It used to have multiple uses, but is no longer needed. Fixing.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/26957) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friend should go first in the private: section, however, I think it would be preferable if we wouldn't use friend (as the styleguide discourages doing so). I don't think there's harm in keeping the ctors public.
Moved friend and ctor declarations as suggested by @jochen. Did not remove friend declarations. Google C++ style guide says: Friends should usually be defined in the same file so that the reader does not have to look in another file to find uses of the private members of a class. A common use of friend is to have a FooBuilder class be a friend of Foo so that it can construct the inner state of Foo correctly, without exposing this state to the world. In some cases it may be useful to make a unittest class a friend of the class it tests. These friend declarations are all in counters.h, and they are there to all constructions/updates without exposing state to the world.
On 2017/06/07 15:42:46, kschimpf wrote: > Moved friend and ctor declarations as suggested by @jochen. > > Did not remove friend declarations. Google C++ style guide says: > > Friends should usually be defined in the same file so that the reader does not > have to look in another file to find uses of the private members of a class. A > common use of friend is to have a FooBuilder class be a friend of Foo so that it > can construct the inner state of Foo correctly, without exposing this state to > the world. In some cases it may be useful to make a unittest class a friend of > the class it tests. > > These friend declarations are all in counters.h, and they are there to all > constructions/updates without exposing state to the world. Sorry for the back and forth - I'm not sure what the value of keeping anything public is, if there is no consumer. I'd prefer we kept the ctors private, for lower maintainability costs. Naturally, should the scenario arise, we'll have a justification to make'em public again.
lgtm - with the nit that i'd prefer keeping ctors private, as originally designed.
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 Link to the patchset: https://codereview.chromium.org/2918703002/#ps160001 (title: "Move ctors back to private.")
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": 160001, "attempt_start_ts": 1496934908615050,
"parent_rev": "fe048410f86f596bd72c0fc456be6021abaa3974", "commit_rev":
"f073a20b6999573a415a3f56912c812d1d737cbe"}
Message was sent while issue was closed.
Description was changed from ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng ========== to ========== Localize counter class member functions. This CL takes advantage of the fact that StatsCounter is now local to the Counters class. This includes: 1) Method StatsTable::SetCreateHistogramFunction() was only called in one spot (in api.cc), which also called Counters::ResetHistograms() and Counters::InitializeHistorgram(). InitializeHistogram can be folded into Histogram.Reset(). 2) Since Histogram::Reset() now regenerats the histogram, we no longer need the field lookup_done_. Therefore there is no longer a race between updating ptr_ and lookup_done_, making the Histogram class thread safe. 3) Made the constructors of several classes private (except for class Counters), minimizing the scope that they are used. When the couldn't be moved, add comment that they were public only for test cases. 4) Removed the need for a mutex lock on StatsCounter::Reset(), since it is now guaranteed to only be called when StatsTable::SetCounterFunction() is called. BUG=v8:6361 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng Review-Url: https://codereview.chromium.org/2918703002 Cr-Commit-Position: refs/heads/master@{#45791} Committed: https://chromium.googlesource.com/v8/v8/+/f073a20b6999573a415a3f56912c812d1d7... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/v8/v8/+/f073a20b6999573a415a3f56912c812d1d7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
