Localize counter class member functions.

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/+/f073a20b6999573a415a3f56912c812d1d737cbe

[kschimpf, 2017-05-31 22:35:03]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 22:35:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 23:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 23:11:24]

Dry run: This issue passed the CQ dry run.

[kschimpf, 2017-06-02 15:18:15]

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
==========

[kschimpf, 2017-06-02 15:19:22]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 15:19:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kschimpf, 2017-06-02 15:22:16]

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, 2017-06-02 15:22:16]

kschimpf@chromium.org changed reviewers:
+ cbruni@chromium.org, clemensh@chromium.org, jochen@chromium.org,
mtrofin@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:04:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 16:04:23]

Dry run: This issue passed the CQ dry run.

[kschimpf, 2017-06-05 14:52:04]

Please review. Thanks!

[Mircea Trofin, 2017-06-05 15:57:49]

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()?

[kschimpf, 2017-06-05 17:38:52]

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.

[kschimpf, 2017-06-05 17:39:02]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 17:39:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 18:00:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 18:00:42]

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...)

[kschimpf, 2017-06-05 18:21:57]

The CQ bit was checked by kschimpf@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 18:22:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 19:09:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-05 19:09:10]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-06-06 14:58:03]

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.

[kschimpf, 2017-06-07 15:42:46]

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.

[Mircea Trofin, 2017-06-07 20:31:47]

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.

[Mircea Trofin, 2017-06-07 20:32:29]

lgtm - with the nit that i'd prefer keeping ctors private, as originally
designed.

[kschimpf, 2017-06-08 15:15:08]

The CQ bit was checked by kschimpf@chromium.org

[kschimpf, 2017-06-08 15:15:09]

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.")

[commit-bot: I haz the power, 2017-06-08 15:15:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 16:18:32]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496934908615050,
"parent_rev": "fe048410f86f596bd72c0fc456be6021abaa3974", "commit_rev":
"f073a20b6999573a415a3f56912c812d1d737cbe"}

[commit-bot: I haz the power, 2017-06-08 16:18:38]

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...
==========

[commit-bot: I haz the power, 2017-06-08 16:18:40]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/f073a20b6999573a415a3f56912c812d1d7...