Oilpan: Remove duplicated code between HeapPage and LargeObject

Oilpan: Remove duplicated code between HeapPage and LargeObject

Basically, this CL changes:

class ThreadHeap {
  ...; // Methods for both HeapPages and LargeObjects
  HeapPage* m_firstPage;
  LargeObject* m_firstLargeObject;
};

to:

class ThreadHeap {
  ...;  // Common methods between HeapPages and LargeObjects.
  BaseHeapPage* m_firstPage;
};
class ThreadHeapForHeapPage {  ...; // Methods for HeapPages. }
class TheadHeapForLargeObject { ...; // Methods for LargeObjects. }

and thus remove various duplicated code between HeapPage and LargeObject. To achieve the goal, this CL creates a dedicated ThreadHeap for LargeObjects (i.e., LargeObjectHeap).

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

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

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

[haraken, 2015-01-09 05:27:53]

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

[haraken, 2015-01-09 05:28:02]

PTAL

[sof, 2015-01-09 10:14:57]

Not convinced this is tidier overall - the presence of many ASSERT()s to check
for large-vs-non-large heaps suggests that the two kinds of object don't really
belong under one (heap) type.

[haraken, 2015-01-09 15:07:25]

On 2015/01/09 10:14:57, sof wrote:
> Not convinced this is tidier overall - the presence of many ASSERT()s to check
> for large-vs-non-large heaps suggests that the two kinds of object don't
really
> belong under one (heap) type.

Yeah, agreed.

Out of curiosity, let me measure performance of this CL.

[haraken, 2015-01-14 02:03:27]

On 2015/01/09 15:07:25, haraken wrote:
> On 2015/01/09 10:14:57, sof wrote:
> > Not convinced this is tidier overall - the presence of many ASSERT()s to
check
> > for large-vs-non-large heaps suggests that the two kinds of object don't
> really
> > belong under one (heap) type.
> 
> Yeah, agreed.
> 
> Out of curiosity, let me measure performance of this CL.

It looks like there is no performance improvement with this CL.
https://docs.google.com/a/google.com/spreadsheets/d/1qFZ5gca9okdtI1ndhw-v-tbU...

(BTW, I'm trying to understand why the performance numbers are so flake even in
super-micro benchmarks like first-child.html.)

[haraken, 2015-01-14 05:12:19]

On 2015/01/14 02:03:27, haraken wrote:
> On 2015/01/09 15:07:25, haraken wrote:
> > On 2015/01/09 10:14:57, sof wrote:
> > > Not convinced this is tidier overall - the presence of many ASSERT()s to
> check
> > > for large-vs-non-large heaps suggests that the two kinds of object don't
> > really
> > > belong under one (heap) type.
> > 
> > Yeah, agreed.
> > 
> > Out of curiosity, let me measure performance of this CL.
> 
> It looks like there is no performance improvement with this CL.
>
https://docs.google.com/a/google.com/spreadsheets/d/1qFZ5gca9okdtI1ndhw-v-tbU...
> 
> (BTW, I'm trying to understand why the performance numbers are so flake even
in
> super-micro benchmarks like first-child.html.)

I investigated the performance of first-child.html (using the sampling profiler
in about:tracing) and noticed that the performance flakiness comes from V8, not
from Blink. In the above spreadsheet, we can observe big flakes in first-child,
undefined-first-child, get-attribute, get-attribute-rare, node-type. The reason
for these big flakes would be because these benchmarks are very light-weight in
the Blink side and thus consuming most of their time in V8.

I'll reach out to the V8 team to see if there is any way to avoid such flakes.

[haraken, 2015-02-02 08:05:09]

Retriggering an old issue...

On 2015/01/09 10:14:57, sof wrote:
> Not convinced this is tidier overall - the presence of many ASSERT()s to check
> for large-vs-non-large heaps suggests that the two kinds of object don't
really
> belong under one (heap) type.

You're right in that ThreadHeap mixes methods for HeapPage and methods for
LargeObjects. The patch set 2 splits ThreadHeap into ThreadHeapForHeapPage and
ThreadHeapForLargeObject to resolve the mess. Does this look nicer?

- I didn't reorder methods in Heap.cpp in this CL to keep the CL size smaller.
- I didn't rename things in this CL to keep the CL size smaller. If we rename
things as follows, the structure will become tidier.

BaseHeapPage, HeapPage, LargeObject => BasePage, NormalPage, LargePage.
ThreadHeap, ThreadHeapForHeapPage, ThreadHeapForLargeObject => BasePageHeap,
NormalPageHeap, LargePageHeap.

[haraken, 2015-02-06 06:50:36]

I might want to land this CL to avoid code duplication in
https://codereview.chromium.org/903033003/, for example.

[sof, 2015-02-06 09:46:52]

lgtm

Consider a followup CL that groups&reorders the methods in Heap.cpp in a more
natural way? (It would make for a less readable CL to do it right here, I
agree.)

https://codereview.chromium.org/840223002/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/840223002/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1405: setAllocationPoint(nullptr, 0);
Why is this here?

[haraken, 2015-02-06 09:59:28]

> Consider a followup CL that groups&reorders the methods in Heap.cpp in a more
natural way?

Sure, will do.

https://codereview.chromium.org/840223002/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/840223002/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1405: setAllocationPoint(nullptr, 0);
On 2015/02/06 09:46:52, sof wrote:
> Why is this here?

Since setAllocationPoint is a method of ThreadHeapForHeapPage (not ThreadHeap),
we cannot keep it in the original place.

I double-checked that calling setAllocationPoint here would be safe.

[haraken, 2015-02-06 10:01:41]

New patchsets have been uploaded after l-g-t-m from sigbjornf@opera.com

[haraken, 2015-02-06 10:01:57]

Checking oilpan try bots.

[haraken, 2015-02-09 04:52:10]

I think I addressed all test failures... Trying bots.

[haraken, 2015-02-09 06:33:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-02-09 06:33:59]

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

[commit-bot: I haz the power, 2015-02-09 06:37:14]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189782

[caseq, 2015-02-09 17:59:52]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/907943002/ by caseq@chromium.org.

The reason for reverting is: This has apparently broken a bunch of chrome side
tests:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....

[haraken, 2015-02-10 02:13:31]

On 2015/02/09 17:59:52, caseq wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/907943002/ by mailto:caseq@chromium.org.
> 
> The reason for reverting is: This has apparently broken a bunch of chrome side
> tests:
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....

It looks like sizeof(HeapPage) is not 8-byte aligned in 32 bit systems and thus
hit the ASSERT.

Uploaded the patch set 6 and trying Linux 32 bots.

[haraken, 2015-02-11 06:29:19]

On 2015/02/10 02:13:31, haraken wrote:
> On 2015/02/09 17:59:52, caseq wrote:
> > A revert of this CL (patchset #5 id:80001) has been created in
> > https://codereview.chromium.org/907943002/ by mailto:caseq@chromium.org.
> > 
> > The reason for reverting is: This has apparently broken a bunch of chrome
side
> > tests:
> >
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....
> 
> It looks like sizeof(HeapPage) is not 8-byte aligned in 32 bit systems and
thus
> hit the ASSERT.
> 
> Uploaded the patch set 6 and trying Linux 32 bots.

I think the patch set 11 addressed the 8-byte alignment issue in 32 bit systems.
Landing.

[haraken, 2015-02-11 06:29:24]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-02-11 06:29:32]

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

[commit-bot: I haz the power, 2015-02-11 06:33:05]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189953

[sof, 2015-02-11 07:03:02]

On 2015/02/11 06:29:19, haraken wrote:
> On 2015/02/10 02:13:31, haraken wrote:
> > On 2015/02/09 17:59:52, caseq wrote:
> > > A revert of this CL (patchset #5 id:80001) has been created in
> > > https://codereview.chromium.org/907943002/ by mailto:caseq@chromium.org.
> > > 
> > > The reason for reverting is: This has apparently broken a bunch of chrome
> side
> > > tests:
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....
> > 
> > It looks like sizeof(HeapPage) is not 8-byte aligned in 32 bit systems and
> thus
> > hit the ASSERT.
> > 
> > Uploaded the patch set 6 and trying Linux 32 bots.
> 
> I think the patch set 11 addressed the 8-byte alignment issue in 32 bit
systems.
> Landing.

lgtm.

(There might be an opportunity to tidy up some of the headerPadding() uses in
Heap.*)

[haraken, 2015-02-11 07:06:06]

On 2015/02/11 07:03:02, sof wrote:
> On 2015/02/11 06:29:19, haraken wrote:
> > On 2015/02/10 02:13:31, haraken wrote:
> > > On 2015/02/09 17:59:52, caseq wrote:
> > > > A revert of this CL (patchset #5 id:80001) has been created in
> > > > https://codereview.chromium.org/907943002/ by mailto:caseq@chromium.org.
> > > > 
> > > > The reason for reverting is: This has apparently broken a bunch of
chrome
> > side
> > > > tests:
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2....
> > > 
> > > It looks like sizeof(HeapPage) is not 8-byte aligned in 32 bit systems and
> > thus
> > > hit the ASSERT.
> > > 
> > > Uploaded the patch set 6 and trying Linux 32 bots.
> > 
> > I think the patch set 11 addressed the 8-byte alignment issue in 32 bit
> systems.
> > Landing.
> 
> lgtm.
> 
> (There might be an opportunity to tidy up some of the headerPadding() uses in
> Heap.*)

Yes, let me do the clean-up in a follow up.

[pdr., 2015-02-11 21:30:14]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/921493003/ by pdr@chromium.org.

The reason for reverting is: This change is being blamed for
win_chromium_x64_rel_ng failures that are blocking the blink roll. See
http://crbug.com/457808 for details..

[haraken, 2015-02-12 02:18:15]

On 2015/02/11 21:30:14, pdr wrote:
> A revert of this CL (patchset #11 id:200001) has been created in
> https://codereview.chromium.org/921493003/ by mailto:pdr@chromium.org.
> 
> The reason for reverting is: This change is being blamed for
> win_chromium_x64_rel_ng failures that are blocking the blink roll. See
> http://crbug.com/457808 for details..

The win_chromium_x64_rel_ng bot looks OK with this CL. Let me reland.

[haraken, 2015-02-12 02:18:19]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2015-02-12 02:19:00]

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

[commit-bot: I haz the power, 2015-02-12 02:19:35]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190016