Fix StatisticsRecorder to handle re-entry during tests.

Fix StatisticsRecorder to handle re-entry during tests.

BUG=592747
Committed: https://crrev.com/b113b974a0cc071bd9822b499975307529eac294
Cr-Commit-Position: refs/heads/master@{#386119}

[bcwhite, 2016-03-08 19:47:19]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[bcwhite, 2016-03-08 19:47:20]

Here's the basic idea of it.  There are still a few things that go wrong with
shuffled tests but I find myself in need of changes in my other CLs to deal with
them gracefully.

[Alexei Svitkine (slow), 2016-03-08 20:04:12]

https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.h:170: scoped_ptr<RangesMap> existing_ranges_;
Can this be done in a test-only subclass?

[bcwhite, 2016-03-08 20:23:27]

https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
File base/metrics/statistics_recorder.h (right):

https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
base/metrics/statistics_recorder.h:170: scoped_ptr<RangesMap> existing_ranges_;
On 2016/03/08 20:04:12, Alexei Svitkine (slow) wrote:
> Can this be done in a test-only subclass?

Potentially.  It'll require turning private members into protected ones.

It's already basically a test-only class except for the one global instance. 
We'll waste 3 pointers worth of memory on that instance.  Still think it's worth
it?

[Alexei Svitkine (slow), 2016-03-08 21:23:14]

I think so - it's not just the memory cost, but also the reducing the
complexity of code running in production (more code = more bugs) - so I
think having the extra functionality be test-only is better, even if it's
simple.

On Tue, Mar 8, 2016 at 3:23 PM, <bcwhite@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
> File base/metrics/statistics_recorder.h (right):
>
>
>
https://codereview.chromium.org/1779503002/diff/1/base/metrics/statistics_rec...
> base/metrics/statistics_recorder.h:170: scoped_ptr<RangesMap>
> existing_ranges_;
> On 2016/03/08 20:04:12, Alexei Svitkine (slow) wrote:
> > Can this be done in a test-only subclass?
>
> Potentially. It'll require turning private members into protected ones.
>
> It's already basically a test-only class except for the one global
> instance. We'll waste 3 pointers worth of memory on that instance.
> Still think it's worth it?
>
> https://codereview.chromium.org/1779503002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2016-03-09 16:48:31]

> I think so - it's not just the memory cost, but also the reducing the
> complexity of code running in production (more code = more bugs) - so I
> think having the extra functionality be test-only is better, even if it's
> simple.

I got part-way down that road but hit a... ummm... roadblock in the constructor.
 The saving of the existing pointers needs to be done before the base
StatisticsRecorder constructor runs (since it writes to those pointers) but
there's no way to make the TestStatisticsRecorder run code in its constructor to
do that extraction before calling the base constructor.

Destruction has the opposite problem in that it needs to run its code after the
base destructor.

If I leave the pointer extraction/saving in StatisticsRecorder then the Test
version of the class reduces to nothing.

I'll keep thinking about it.

[bcwhite, 2016-03-18 01:24:30]

Patchset #4 (id:60001) has been deleted

[bcwhite, 2016-03-18 01:24:48]

Patchset #4 (id:80001) has been deleted

[bcwhite, 2016-04-07 17:32:55]

Patchset #4 (id:100001) has been deleted

[bcwhite, 2016-04-07 17:33:05]

Patchset #4 (id:120001) has been deleted

[bcwhite, 2016-04-07 17:33:14]

Patchset #4 (id:140001) has been deleted

[bcwhite, 2016-04-07 17:33:24]

Patchset #4 (id:160001) has been deleted

[bcwhite, 2016-04-07 17:33:37]

Patchset #4 (id:180001) has been deleted

[bcwhite, 2016-04-07 17:41:09]

On 2016/03/09 16:48:31, bcwhite wrote:
> > I think so - it's not just the memory cost, but also the reducing the
> > complexity of code running in production (more code = more bugs) - so I
> > think having the extra functionality be test-only is better, even if it's
> > simple.
> 
> I got part-way down that road but hit a... ummm... roadblock in the
constructor.
>  The saving of the existing pointers needs to be done before the base
> StatisticsRecorder constructor runs (since it writes to those pointers) but
> there's no way to make the TestStatisticsRecorder run code in its constructor
to
> do that extraction before calling the base constructor.
> 
> Destruction has the opposite problem in that it needs to run its code after
the
> base destructor.
> 
> If I leave the pointer extraction/saving in StatisticsRecorder then the Test
> version of the class reduces to nothing.
> 
> I'll keep thinking about it.

I don't think I can move the save-pointers code out to a Test class but I could
create one that contains only the UninitializeForTesting method.

[bcwhite, 2016-04-08 13:23:57]

Patchset #4 (id:200001) has been deleted

[Alexei Svitkine (slow), 2016-04-08 15:04:19]

lgtm

https://codereview.chromium.org/1779503002/diff/220001/base/metrics/statistic...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/1779503002/diff/220001/base/metrics/statistic...
base/metrics/statistics_recorder_unittest.cc:68:
StatisticsRecorder::UninitializeForTesting();
Shouldn't this be part of UninitializeStatisticsRecorder()?

[bcwhite, 2016-04-08 16:06:33]

https://codereview.chromium.org/1779503002/diff/220001/base/metrics/statistic...
File base/metrics/statistics_recorder_unittest.cc (right):

https://codereview.chromium.org/1779503002/diff/220001/base/metrics/statistic...
base/metrics/statistics_recorder_unittest.cc:68:
StatisticsRecorder::UninitializeForTesting();
On 2016/04/08 15:04:19, Alexei Svitkine wrote:
> Shouldn't this be part of UninitializeStatisticsRecorder()?

It's not generally important since it's just a stack of SRs now.  This test,
however, specifically tests the functionality when that stack is empty so it was
necessary.

Can't see any harm in doing it all the time, though, so Done.

[bcwhite, 2016-04-08 17:31:52]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-04-08 17:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1779503002/#ps240001
(title: "always do global UninitializeForTesting when uninitializing")

[commit-bot: I haz the power, 2016-04-08 17:32:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779503002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779503002/240001

[commit-bot: I haz the power, 2016-04-08 17:38:42]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2016-04-08 17:40:13]

Description was changed from

==========
Fix StatisticsRecorder to handle re-entry during tests.

BUG=592747
==========

to

==========
Fix StatisticsRecorder to handle re-entry during tests.

BUG=592747

Committed: https://crrev.com/b113b974a0cc071bd9822b499975307529eac294
Cr-Commit-Position: refs/heads/master@{#386119}
==========

[commit-bot: I haz the power, 2016-04-08 17:40:14]

Patchset 5 (id:??) landed as
https://crrev.com/b113b974a0cc071bd9822b499975307529eac294
Cr-Commit-Position: refs/heads/master@{#386119}