Introduce HeapAllocHooks

Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a

Committed: https://crrev.com/a81fa2467a3277849b423df3622f33c852665118
Cr-Commit-Position: refs/heads/master@{#375424}

[hajimehoshi, 2016-02-08 12:34:56]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org

[hajimehoshi, 2016-02-08 12:34:56]

PTAL

[hajimehoshi, 2016-02-08 12:45:20]

Description was changed from

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement heap profiler first and add type names
later.

BUG=585063
TEST=n/a
==========

to

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a
==========

[haraken, 2016-02-08 14:35:51]

This is adding a branch to a performance-sensitive path. Would you verify that
this CL doesn't regress blink_perf.* and dromaeo.*?

[hajimehoshi, 2016-02-10 10:50:57]

hajimehoshi@chromium.org changed reviewers:
+ tasak@google.com

[hajimehoshi, 2016-02-10 10:51:03]

On 2016/02/08 14:35:51, haraken wrote:
> This is adding a branch to a performance-sensitive path. Would you verify that
> this CL doesn't regress blink_perf.* and dromaeo.*?

As long as I read the results of blink_perf.*, there seems to be some
regressions (-18% at maximum) in some cases, but I think they are reliable
results. It's because 1) the results of experiments on my local machine are
flaky (I did blink_perf.* tests twice), and 2) in theory, all those test HTMLs
should execute almost same numbers of allocation and the test results should be
same. Actually, I counted how many times allocation are called and the numbers
are almost same. As far as I know, there is no test that allocates objects
aggressively in blink_perf.*.

I'll share you with results.html files later.

I'll test tonight again and I asked tasak-san to do the same thing, but I expect
we would get the same result.

[hajimehoshi, 2016-02-10 11:08:32]

On 2016/02/10 10:51:03, hajimehoshi wrote:
> On 2016/02/08 14:35:51, haraken wrote:
> > This is adding a branch to a performance-sensitive path. Would you verify
that
> > this CL doesn't regress blink_perf.* and dromaeo.*?
> 
> As long as I read the results of blink_perf.*, there seems to be some
> regressions (-18% at maximum) in some cases, but I think they are reliable
> results. It's because 1) the results of experiments on my local machine are
> flaky (I did blink_perf.* tests twice), and 2) in theory, all those test HTMLs
> should execute almost same numbers of allocation and the test results should
be
> same. Actually, I counted how many times allocation are called and the numbers
> are almost same. As far as I know, there is no test that allocates objects
> aggressively in blink_perf.*.
> 
> I'll share you with results.html files later.
> 
> I'll test tonight again and I asked tasak-san to do the same thing, but I
expect
> we would get the same result.

> in theory, all those test HTMLs should execute almost same numbers of
allocation and the test results shoulbe same. Actually, I counted how many times
allocation are called and the numbers are almost same. As far as I know, there
is no test that allocates objects aggressively in blink_perf.*.

Sorry but I didn't check create-element.html, which should calls many
allocations. However, create-element.html caused only a little regression, and
document-implementation.html, which should NOT call many allocations, caused
bigger regression than create-element. I can't reason about this situation and I
think they are just flakes.

[haraken, 2016-02-12 10:29:19]

tasak's measurement showed no regression in blink_perf.bindings,
blink_perf.parser and dromaeo.

LGTM.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:60: if (UNLIKELY(allocationHook
!= nullptr))

UNLIKELY(!allocationHook)

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:67: if (UNLIKELY(freeHook !=
nullptr))

UNLIKELY(!freeHook)

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:84: // free if the pointers are
non-null.

I'd remove this comment.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1136:
HeapAllocHooks::freeHookIfEnabled(payload);

Can we move this into header->finalize()?

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452:
HeapAllocHooks::freeHookIfEnabled(payload());

Then you can remove this.

[hajimehoshi, 2016-02-15 07:14:46]

Thank you!

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:60: if (UNLIKELY(allocationHook
!= nullptr))
On 2016/02/12 10:29:19, haraken wrote:
> 
> UNLIKELY(!allocationHook)

I guess UNLIKELY(!!allocationHook). Done.

UNLIKELY(allocationHook) didn't work BTW.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:67: if (UNLIKELY(freeHook !=
nullptr))
On 2016/02/12 10:29:18, haraken wrote:
> 
> UNLIKELY(!freeHook)

UNLIKELY(!!freeHook). Done.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/Heap.h:84: // free if the pointers are
non-null.
On 2016/02/12 10:29:19, haraken wrote:
> 
> I'd remove this comment.

Done.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/heap/HeapPage.cpp (right):

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1136:
HeapAllocHooks::freeHookIfEnabled(payload);
On 2016/02/12 10:29:19, haraken wrote:
> 
> Can we move this into header->finalize()?

Done.

https://codereview.chromium.org/1676973002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/heap/HeapPage.cpp:1452:
HeapAllocHooks::freeHookIfEnabled(payload());
On 2016/02/12 10:29:19, haraken wrote:
> 
> Then you can remove this.

Done.

[hajimehoshi, 2016-02-15 07:14:50]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-02-15 07:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1676973002/#ps60001
(title: "haraken's review")

[commit-bot: I haz the power, 2016-02-15 07:15:39]

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

[commit-bot: I haz the power, 2016-02-15 08:34:00]

Description was changed from

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a
==========

to

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a
==========

[commit-bot: I haz the power, 2016-02-15 08:34:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-16 22:49:09]

Description was changed from

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a
==========

to

==========
Introduce HeapAllocHooks

This CL introduces HeapAllocHooks like WTF::PartitionAllocHooks. This is
preparation for Oilpan heap profiler on memory infra.

Now hooks doesn't accept object type names. This would require a lot of
code change, so I'll implement fundamental features for heap profiler
first and add type names later.

BUG=585063
TEST=n/a

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

[commit-bot: I haz the power, 2016-02-16 22:49:09]

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