Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker.

Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly starting and stopping on creation/destruction. This is necessary to support exclusive scopes, which is how base/tracked_objects.h does task execution time measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes, as here must always be a gap from reading the current state to when the Scoped* instance goes out of scope. Any heap activity in this gap will be lost from measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is enabled. It is now allowed to create ThreadHeapTracker instances without any initialization, though it's not allowed to Start() them.

BUG=644385
Committed: https://crrev.com/07856a7b8add0eba66d71fc62b5e32be9bc03d69
Cr-Commit-Position: refs/heads/master@{#425997}

[Sigurður Ásgeirsson, 2016-10-17 18:07:17]

Description was changed from

==========
Rename ScopeThreadHeapUsage to ThreadHeapUsageTracker and change interface.

BUG=644385
==========

to

==========
Rename ScopeThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

[Sigurður Ásgeirsson, 2016-10-17 18:07:17]

siggi@chromium.org changed reviewers:
+ primiano@chromium.org

[Sigurður Ásgeirsson, 2016-10-17 18:08:38]

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

[commit-bot: I haz the power, 2016-10-17 18:09:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2016-10-17 18:09:36]

Hey Primiano,

here's the CL I threatened before the weekend.

Siggi

[Sigurður Ásgeirsson, 2016-10-17 18:10:01]

Description was changed from

==========
Rename ScopeThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

to

==========
Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

[commit-bot: I haz the power, 2016-10-17 18:15:18]

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

[commit-bot: I haz the power, 2016-10-17 18:15:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Sigurður Ásgeirsson, 2016-10-17 19:16:31]

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

[commit-bot: I haz the power, 2016-10-17 19:17:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrisha, 2016-10-17 19:17:46]

chrisha@chromium.org changed reviewers:
+ chrisha@chromium.org

[chrisha, 2016-10-17 19:17:48]

Tests look nice and thorough!

lgtm other than nits

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:30: // Where available, cumulative number
of heap padding heap
heap padding heap?

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:72: // outer scope's usage. If
|usage_is_exclusive" is false, the usage tallied
|usage_is_exclusive"

[Sigurður Ásgeirsson, 2016-10-17 19:33:38]

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

[commit-bot: I haz the power, 2016-10-17 19:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2016-10-17 19:35:22]

siggi@chromium.org changed reviewers:
+ thestig@chromium.org

[Sigurður Ásgeirsson, 2016-10-17 19:35:23]

Thanks - thestig@chromium.org for owners pretty please?

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:30: // Where available, cumulative number
of heap padding heap
On 2016/10/17 19:17:48, chrisha (slow) wrote:
> heap padding heap?

Done.

https://codereview.chromium.org/2427503003/diff/100001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:72: // outer scope's usage. If
|usage_is_exclusive" is false, the usage tallied
On 2016/10/17 19:17:48, chrisha (slow) wrote:
> |usage_is_exclusive"

Done.

[Lei Zhang, 2016-10-17 20:54:23]

lgtm

https://codereview.chromium.org/2427503003/diff/120001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2427503003/diff/120001/base/BUILD.gn#newcode269
base/BUILD.gn:269: # allocator.gyp for details).
I know you are just moving this, but the GYP file no longer exists.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.cc (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:164: if (thread_usage_ != nullptr) {
Do you need the " != nullptr" bit?

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:41: // The maximal value of alloc_bytes -
free_bytes seen for this thread.
nit: |alloc_bytes|, |free_bytes|

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:66: // Note |IsHeapTrackingEnabled()|
must be true.
And IsHeapTrackingEnabled() is not a variable.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker_unittest.cc (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker_unittest.cc:22: using
ThreadHeapUsageTracker::EnsureTLSInitializedForTesting;
nit: Alphabetical order.

[commit-bot: I haz the power, 2016-10-17 22:06:26]

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

[commit-bot: I haz the power, 2016-10-17 22:06:26]

Dry run: Exceeded global retry quota

[Sigurður Ásgeirsson, 2016-10-18 13:39:23]

Thanks!

https://codereview.chromium.org/2427503003/diff/120001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2427503003/diff/120001/base/BUILD.gn#newcode269
base/BUILD.gn:269: # allocator.gyp for details).
On 2016/10/17 20:54:23, Lei Zhang wrote:
> I know you are just moving this, but the GYP file no longer exists.

Done, thanks.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.cc (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:164: if (thread_usage_ != nullptr) {
On 2016/10/17 20:54:23, Lei Zhang wrote:
> Do you need the " != nullptr" bit?

I write out pointer tests in full elsewhere in the file, so the same here for
consistency.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:41: // The maximal value of alloc_bytes -
free_bytes seen for this thread.
On 2016/10/17 20:54:23, Lei Zhang wrote:
> nit: |alloc_bytes|, |free_bytes|

Done.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:66: // Note |IsHeapTrackingEnabled()|
must be true.
On 2016/10/17 20:54:23, Lei Zhang wrote:
> And IsHeapTrackingEnabled() is not a variable.

Done.

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker_unittest.cc (right):

https://codereview.chromium.org/2427503003/diff/120001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker_unittest.cc:22: using
ThreadHeapUsageTracker::EnsureTLSInitializedForTesting;
On 2016/10/17 20:54:23, Lei Zhang wrote:
> nit: Alphabetical order.

Done.

[Sigurður Ásgeirsson, 2016-10-18 13:39:28]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2016-10-18 13:39:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrisha@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2427503003/#ps140001
(title: "Address Lei Zhang's comments, fix leak in test.")

[commit-bot: I haz the power, 2016-10-18 13:39:43]

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, 2016-10-18 14:51:08]

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

[commit-bot: I haz the power, 2016-10-18 14:51:09]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sigurður Ásgeirsson, 2016-10-18 14:54:59]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2016-10-18 14:55:14]

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

[Primiano Tucci (use gerrit), 2016-10-18 15:11:14]

AAAAAAAAA this is back
"hello, who is proof of concept? My name is official CL ready to land" :P
(insert that homer meme here)

LGTM with very minor comments

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.cc (right):

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:186: ThreadHeapUsage current =
*thread_usage_;
dcheck this isn't null (user didn't stop twice()) ?

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:245: void
ThreadHeapUsageTracker::EnsureTLSInitializedForTesting() {
uh why not calling the real one from tests then?

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:73: void Stop(bool usage_is_exclusive);
maybe mention that this is expected to be called on the same thread where
Start() was called, otherwise an army of zombies will come our from the nearest
router and eat your workstation.

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:80: static ThreadHeapUsage
CurrentUsage();
I see the difference between these two, but the two names "usage" vs
"currentusage" are a bit too close to each other and confusing.
maybe s/usage/usage_for_last_scope/ and s/CurrentUsage/GetUsageSnapshot/ (or
SnapshotUsage() or something like that)?

[Sigurður Ásgeirsson, 2016-10-18 15:15:41]

The CQ bit was unchecked by siggi@chromium.org

[Sigurður Ásgeirsson, 2016-10-18 15:48:40]

Thanks, now committing for realz, eh!

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.cc (right):

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:186: ThreadHeapUsage current =
*thread_usage_;
On 2016/10/18 15:11:13, Primiano Tucci wrote:
> dcheck this isn't null (user didn't stop twice()) ?

Done.

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.cc:245: void
ThreadHeapUsageTracker::EnsureTLSInitializedForTesting() {
On 2016/10/18 15:11:13, Primiano Tucci wrote:
> uh why not calling the real one from tests then? 

Good point, done.

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
File base/debug/thread_heap_usage_tracker.h (right):

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:73: void Stop(bool usage_is_exclusive);
On 2016/10/18 15:11:13, Primiano Tucci wrote:
> maybe mention that this is expected to be called on the same thread where
> Start() was called, otherwise an army of zombies will come our from the
nearest
> router and eat your workstation.

Done.

https://codereview.chromium.org/2427503003/diff/140001/base/debug/thread_heap...
base/debug/thread_heap_usage_tracker.h:80: static ThreadHeapUsage
CurrentUsage();
On 2016/10/18 15:11:14, Primiano Tucci wrote:
> I see the difference between these two, but the two names "usage" vs
> "currentusage" are a bit too close to each other and confusing.
> maybe s/usage/usage_for_last_scope/ and s/CurrentUsage/GetUsageSnapshot/ (or
> SnapshotUsage() or something like that)?

Done.

[Sigurður Ásgeirsson, 2016-10-18 15:48:45]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2016-10-18 15:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, thestig@chromium.org, chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2427503003/#ps160001
(title: "Addressed Primiano's nits.")

[commit-bot: I haz the power, 2016-10-18 15:49:08]

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, 2016-10-18 17:18:51]

Description was changed from

==========
Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

to

==========
Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

[commit-bot: I haz the power, 2016-10-18 17:18:52]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-18 17:20:31]

Description was changed from

==========
Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385
==========

to

==========
Rename ScopedThreadHeapUsage to ThreadHeapUsageTracker and change interface.

This is broken out from https://codereview.chromium.org/2386123003/, and the
interface changes are influenced by actual usage.

The interface is changed to use explicit Start/Stop, instead of implicitly
starting and stopping on creation/destruction. This is necessary to support
exclusive scopes, which is how base/tracked_objects.h does task execution time
measurement. Unfortunately a Scoped* abstraction can't support exclusive scopes,
as here must always be a gap from reading the current state to when the Scoped*
instance goes out of scope. Any heap activity in this gap will be lost from
measurement, and will neither tally to the current nor the outer scope.

Initialization is also changed, such that TLS is initialized as heap tracking is
enabled. It is now allowed to create ThreadHeapTracker instances without any
initialization, though it's not allowed to Start() them.

BUG=644385

Committed: https://crrev.com/07856a7b8add0eba66d71fc62b5e32be9bc03d69
Cr-Commit-Position: refs/heads/master@{#425997}
==========

[commit-bot: I haz the power, 2016-10-18 17:20:33]

Patchset 9 (id:??) landed as
https://crrev.com/07856a7b8add0eba66d71fc62b5e32be9bc03d69
Cr-Commit-Position: refs/heads/master@{#425997}