Oilpan: Simplify Heap::allocate

Oilpan: Simplify Heap::allocate

This CL simplifies Heap::allocate for an upcoming optimization (I'm planning to implement size-aware Heap). This CL doesn't improve performance.

- The key change is in Heap::allocate.

- After this CL, ThreadState::getStats and ThreadState::getStatsForTesting return different stats. ThreadState::getStats returns the size of allocated objects *including* object headers, whereas ThreadState::getStatsForTesting returns the size of allocated objects *excluding* object headers. This is because there is no easy way to know the header size of each object in tests, and thus tests cannot test the size of allocated objects unless ThreadState::getStatsForTesting returns the size of allocated objects *excluding* object headers.

- This CL renames Heap::getStats to Heap::getStatsForTesting because it is used by testing only.

BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184285

[haraken, 2014-10-12 08:36:41]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2014-10-12 08:36:46]

PTAL

[sof, 2014-10-13 08:08:46]

Some failures to address before it is ready?

[Erik Corry, 2014-10-13 13:20:32]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2014-10-13 13:20:32]

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1410: size_t allocationSize = size + sizeof(Header);
You can't move this above the release assert, because this addition could wrap
around, and then the release assert would pass, but the allocation would be
tiny.  This is potentially a huge security issue!

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1453: BaseHeap* heap =
state->heap(gcInfo->heapIndex());
Why is this faster?

The HeapTraits should be known at code generation time, and
gcInfo->hasFinalizer() is just as known as gcInfo->heapIndex().

[haraken, 2014-10-14 10:36:09]

keishi@: I fixed all crashes in the patch set 3, but at the same time I noticed
that the patch set 3 doesn't improve benchmark performance in Linux desktop. Can
you try the patch set 3 on Mac and see if something gets better?

[haraken, 2014-10-14 10:43:08]

This CL is not yet for review. The CL fixes all crashes but I cannot observe any
performance improvement and I'm not sure if this optimization is a worth or not.

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1410: size_t allocationSize = size + sizeof(Header);
On 2014/10/13 13:20:32, Erik Corry wrote:
> You can't move this above the release assert, because this addition could wrap
> around, and then the release assert would pass, but the allocation would be
> tiny.  This is potentially a huge security issue!

Thanks for catching this, done.

https://codereview.chromium.org/634243004/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1453: BaseHeap* heap =
state->heap(gcInfo->heapIndex());
On 2014/10/13 13:20:32, Erik Corry wrote:
> Why is this faster?
> 
> The HeapTraits should be known at code generation time, and
> gcInfo->hasFinalizer() is just as known as gcInfo->heapIndex().

You're right. I confirmed that the assembly is the same. Removed.

[keishi, 2014-10-20 04:54:46]

On 2014/10/14 10:36:09, haraken wrote:
> keishi@: I fixed all crashes in the patch set 3, but at the same time I
noticed
> that the patch set 3 doesn't improve benchmark performance in Linux desktop.
Can
> you try the patch set 3 on Mac and see if something gets better?

I re-measured using my office Mac Pro but I still couldn't get significant
performance gains.
I added the data to the "Oct 20" sheet.
https://docs.google.com/a/chromium.org/spreadsheets/d/1R7PmKDvGgMO9rDsBMyCQMP...
I spent the weekend double checking my statistics calculations. I also
calculated the required sample size and I think 50 samples is sufficient to
detect >1% change.

[haraken, 2014-10-20 05:02:03]

On 2014/10/20 04:54:46, keishi wrote:
> On 2014/10/14 10:36:09, haraken wrote:
> > keishi@: I fixed all crashes in the patch set 3, but at the same time I
> noticed
> > that the patch set 3 doesn't improve benchmark performance in Linux desktop.
> Can
> > you try the patch set 3 on Mac and see if something gets better?
> 
> I re-measured using my office Mac Pro but I still couldn't get significant
> performance gains.
> I added the data to the "Oct 20" sheet.
>
https://docs.google.com/a/chromium.org/spreadsheets/d/1R7PmKDvGgMO9rDsBMyCQMP...
> I spent the weekend double checking my statistics calculations. I also
> calculated the required sample size and I think 50 samples is sufficient to
> detect >1% change.

Thanks for the detailed verification! Then this CL won't be helpful for
performance (although it's a good thing to make Heap::allocate slimmer in
general)...

[haraken, 2014-10-23 04:20:50]

PTAL

Although this CL doesn't improve performance, I'm going to land this CL to clean
up Heap::allocate for an upcoming optimization (I'm planning to implement
per-size Heap).

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
File Source/platform/heap/HeapTest.cpp (left):

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
Source/platform/heap/HeapTest.cpp:1617: // Endless loop unless we eventually get
the memory back that we just freed.

Removed this test because the test result depends on how the allocator reuses
the memory address space and thus is not deterministic.

[Erik Corry, 2014-10-23 07:18:36]

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1364: if (LIKELY(nextAllocationPoint <=
m_allocationLimit)) {
I'm not sure this change is OK.  allocationSize can be very large, up to
maxHeapObjectSize.  There could be a heap page close to the top of the memory
space, and the nextAllocation Point would then wrap around to the start of
memory and this "if" would erroneously be true.

It was the intention of the old code that this allocate function would often be
inlined with a constant "size", in which case the "if (largeObject)" would
disappear after constant propagation.  I don't know whether the inliner is eager
enough for this in practice.

I personally feel that this function is prettier with an early "if (...) return
..." instead of having everything in an "if" like this.  That's just taste
though, it's up to you.

[haraken, 2014-10-23 08:37:53]

Thanks for review! PTAL.

In the end, this is a CL that just made Gustav's original CL pass tests.

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/190001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1364: if (LIKELY(nextAllocationPoint <=
m_allocationLimit)) {
On 2014/10/23 07:18:36, Erik Corry wrote:
> I'm not sure this change is OK.  allocationSize can be very large, up to
> maxHeapObjectSize.  There could be a heap page close to the top of the memory
> space, and the nextAllocation Point would then wrap around to the start of
> memory and this "if" would erroneously be true.

Done.


> I personally feel that this function is prettier with an early "if (...)
return
> ..." instead of having everything in an "if" like this.  That's just taste
> though, it's up to you.

I'd prefer the new style since it removes # of branches from the fast path,
although that didn't affect performance :)

[Erik Corry, 2014-10-23 09:29:34]

lgtm

https://codereview.chromium.org/634243004/diff/210001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/210001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1372: m_currentAllocationPoint =
m_currentAllocationPoint + allocationSize;
This should use +=

[haraken, 2014-10-23 11:29:59]

https://codereview.chromium.org/634243004/diff/210001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/634243004/diff/210001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1372: m_currentAllocationPoint =
m_currentAllocationPoint + allocationSize;
On 2014/10/23 09:29:34, Erik Corry wrote:
> This should use +=

Done.

[haraken, 2014-10-23 11:30:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-10-23 11:31:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634243004/230001

[commit-bot: I haz the power, 2014-10-23 16:51:47]

Committed patchset #5 (id:230001) as 184285