heap-profiler: Clean up some comments and C++.

heap-profiler: Clean up some comments and C++.

BUG=698079
Review-Url: https://codereview.chromium.org/2797603002
Cr-Commit-Position: refs/heads/master@{#461817}
Committed: https://chromium.googlesource.com/chromium/src/+/7cf40cf83838145d2e6310f4f923f316b859e4f1

[awong, 2017-04-04 00:19:16]

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

[commit-bot: I haz the power, 2017-04-04 00:19:43]

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

[awong, 2017-04-04 00:20:41]

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

[commit-bot: I haz the power, 2017-04-04 02:57:27]

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

[commit-bot: I haz the power, 2017-04-04 02:57:28]

Dry run: 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_...)

[Primiano Tucci (use gerrit), 2017-04-04 09:50:24]

LGTM.

Can you plz add just a little bit more context to the CL title, e.g.:

heap-profiler: Clean up some comments and C++.

As a perf-sheriff, when I scroll though 200 commits per perf bug, I like to be
able to tell which part of the code a CL is about, without having to click on
that.

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_register.cc (right):

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_register.cc:81:
out_of_storage_backtrace_index_(CreateBacktraceSentinel()) {}
awesome. I wanted to suggest this yesterday during our chat, but that didn't
want to cause too much trouble.
Along these lines, how about at this point:
- make const kOutOfStorageBackTraceIndex = 0;
- in this ctor just:
  KVIndex sentinel_index = CreateBacktraceSentinel();
  DCHECK_EQ(kOutOfStorageBackTraceIndex, sentinel_index)
 (although at this point you could argue there is no need for a dedicated
CreateBacktraceSentinel function anymore that that could be just in the body of
the ctor)

This should allow some more const propagation and emit code which is hopefully
be faster and smaller than the current one.
(having said this, not too strong on this, so feel free to keep the code as it
is if you prefer this way)

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_register.h (right):

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_register.h:32: // TODO(awong): Remove
the guarded memory. This isn't currently being used and
>  This isn't currently being used 
You say "this isn't currently used" but that is what allowed me to spot the
previous bug about the sentinel refcounting..

> this code is complex looking enough
true, but how is removing the guard page making anything less complex? You talk
as if by removing the guard page we could make this hashtable, or the logic
around that, way simpler. unless I am missing something, that's not true.

[awong, 2017-04-04 18:05:36]

Description was changed from

==========
Clean up some comments and C++.

BUG=698079
==========

to

==========
Heap profiler: Clean up some comments and C++.

BUG=698079
==========

[awong, 2017-04-04 18:05:55]

Description was changed from

==========
Heap profiler: Clean up some comments and C++.

BUG=698079
==========

to

==========
heap-profiler: Clean up some comments and C++.

BUG=698079
==========

[awong, 2017-04-04 18:28:25]

The CQ bit was checked by ajwong@chromium.org

[awong, 2017-04-04 18:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2797603002/#ps20001
(title: "Enum are great")

[awong, 2017-04-04 18:28:30]

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_register.cc (right):

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_register.cc:81:
out_of_storage_backtrace_index_(CreateBacktraceSentinel()) {}
On 2017/04/04 09:50:23, Primiano Tucci wrote:
> awesome. I wanted to suggest this yesterday during our chat, but that didn't
> want to cause too much trouble.
> Along these lines, how about at this point:
> - make const kOutOfStorageBackTraceIndex = 0;
> - in this ctor just:
>   KVIndex sentinel_index = CreateBacktraceSentinel();
>   DCHECK_EQ(kOutOfStorageBackTraceIndex, sentinel_index)
>  (although at this point you could argue there is no need for a dedicated
> CreateBacktraceSentinel function anymore that that could be just in the body
of
> the ctor)
> 
> This should allow some more const propagation and emit code which is hopefully
> be faster and smaller than the current one.
> (having said this, not too strong on this, so feel free to keep the code as it
> is if you prefer this way)

Done.

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_register.h (right):

https://codereview.chromium.org/2797603002/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_register.h:32: // TODO(awong): Remove
the guarded memory. This isn't currently being used and
On 2017/04/04 09:50:23, Primiano Tucci wrote:
> >  This isn't currently being used 
> You say "this isn't currently used" but that is what allowed me to spot the
> previous bug about the sentinel refcounting..
> 
> > this code is complex looking enough
> true, but how is removing the guard page making anything less complex? You
talk
> as if by removing the guard page we could make this hashtable, or the logic
> around that, way simpler. unless I am missing something, that's not true.

I'll remove it.  The code just has too many concepts in one file. That's yet
another thing to think through and drift. For example, we had a logic divergence
where the code comment talked about relying on guard pages to crash and then
someone added a CHECK fail later. I agree it's benign, but I would prefer
removing
it. On the otherhand, this discussion has already pushed me beyond how much I
care to discuss it.

[commit-bot: I haz the power, 2017-04-04 18:29:21]

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-04-04 20:31:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491330505441900,
"parent_rev": "ce7fb95574ad8c33b9cf382bb133769199c72563", "commit_rev":
"7cf40cf83838145d2e6310f4f923f316b859e4f1"}

[commit-bot: I haz the power, 2017-04-04 20:32:15]

Description was changed from

==========
heap-profiler: Clean up some comments and C++.

BUG=698079
==========

to

==========
heap-profiler: Clean up some comments and C++.

BUG=698079

Review-Url: https://codereview.chromium.org/2797603002
Cr-Commit-Position: refs/heads/master@{#461817}
Committed:
https://chromium.googlesource.com/chromium/src/+/7cf40cf83838145d2e6310f4f923...
==========

[commit-bot: I haz the power, 2017-04-04 20:32:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/7cf40cf83838145d2e6310f4f923...