Oilpan: move RenderObjects off heap.

Oilpan: move RenderObjects off heap.

Have the RenderObject hierarchy no longer be Oilpan heap allocated,
keeping its objects allocated via PartitionAlloc when Oilpan is
enabled also.

No separate & conditional code path is provided for "backwards
compatibility" of re-enabling Oilpan for these objects.

R=haraken,tkent
BUG=398342
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189496

[sof, 2015-02-03 19:43:58]

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

[sof, 2015-02-03 19:44:00]

Please take a look.

[haraken, 2015-02-03 23:28:37]

Thanks!!! Let me take a detailed look.

[tkent, 2015-02-04 00:50:43]

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

[tkent, 2015-02-04 00:50:44]

lgtm

https://codereview.chromium.org/869323003/diff/20001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimationsTest.cpp (left):

https://codereview.chromium.org/869323003/diff/20001/Source/core/animation/Co...
Source/core/animation/CompositorAnimationsTest.cpp:264: static void
dispose(PassOwnPtrWillBeRawPtr<RenderObjectProxy> object)
Don't remove this function.  This is not specific to Oilpan, and we need to call
RenderObject::destroy().

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:323: #if ENABLE(OILPAN)
We can remove this block.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:61: static
OwnPtr<ContinuationMap>* continuationMap = 0;
This should be |ContinuationMap* continuationMap = nullptr|. We don't need to
use OwnPtr.
Also, (*continuationMap)->foo() should be continuationMap->foo().

[haraken, 2015-02-04 05:48:54]

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

[haraken, 2015-02-04 05:48:56]

Mostly looks good.

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:92: ALLOW_ONLY_INLINE_ALLOCATION();

You can drop ALLOW_ONLY_INLINE_ALLOCATION().

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:118: ALLOW_ONLY_INLINE_ALLOCATION();

You can drop ALLOW_ONLY_INLINE_ALLOCATION().

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:61: static
OwnPtr<ContinuationMap>* continuationMap = 0;
On 2015/02/04 00:50:44, tkent wrote:
> This should be |ContinuationMap* continuationMap = nullptr|. We don't need to
> use OwnPtr.
> Also, (*continuationMap)->foo() should be continuationMap->foo().
> 

Can we use DEFINE_STATIC_LOCAL?

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:669: ALLOW_ONLY_INLINE_ALLOCATION();

You can drop ALLOW_ONLY_INLINE_ALLOCATION().

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:250: ASSERT(m_didCallDestroy);

Can we keep m_didCallDestroy? I think RenderObject::destroy() needs to be called
in both oilpan and non-oilpan even after this CL.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:129: class RenderObject : public
ImageResourceClient {

tkent-san: I remember we moved RenderObject to Oilpan's heap to avoid
introducing Persistent handles in hot call paths (when we moved the Resource
hierarchy to Oilpan's heap). However, I don't see any newly added Persistent
handle in this CL. Do you remember why we had to move RenderObject to Oilpan's
heap and why the issue is gone now?

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderPart.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderPart.cpp:55: if (--m_refCount <= 0)

Not related to this CL, it would be better to add:

  ASSERT(m_refCount >= 0);

and change the condition to if(!m_refCount).

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderPart.cpp:84: ASSERT(m_refCount <= 0);

Not related to this CL, ASSERT(!m_refCount) would be better.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTextFragment.h:85: // Oilpan: weak reference back to
GCed owner; its lifetime subsumes this object.

I'm not sure of this comment. Even if RenderObject is on-heap, I guess
m_firstLetterPseudoElement should be a strong reference (i.e., Member).

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
Persistent<PatternAttributesWrapper> m_attributesWrapper;

Can we avoid introducing a wrapper by making PatternAttributes not-a-part-of
object in both oilpan and non-oilpan builds?

[tkent, 2015-02-04 06:07:47]

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:129: class RenderObject : public
ImageResourceClient {
On 2015/02/04 05:48:56, haraken wrote:
> 
> tkent-san: I remember we moved RenderObject to Oilpan's heap to avoid
> introducing Persistent handles in hot call paths (when we moved the Resource
> hierarchy to Oilpan's heap). However, I don't see any newly added Persistent
> handle in this CL. Do you remember why we had to move RenderObject to Oilpan's
> heap and why the issue is gone now?

My plan was to make ImageResourceClient GCMixin after moving RenderObject to
Oilpan heap for [1].  But I suspended this task fortunately.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[tkent, 2015-02-04 06:09:48]

> BUG=340522

I have updated this to BUG=398342.

[sof, 2015-02-04 06:11:27]

On 2015/02/04 06:09:48, tkent wrote:
> > BUG=340522
> 
> I have updated this to BUG=398342.

Many thanks, that's the appropriate one to use.

[sof, 2015-02-04 08:42:22]

New patchsets have been uploaded after l-g-t-m from tkent@chromium.org

[sof, 2015-02-04 08:44:32]

https://codereview.chromium.org/869323003/diff/20001/Source/core/animation/Co...
File Source/core/animation/CompositorAnimationsTest.cpp (left):

https://codereview.chromium.org/869323003/diff/20001/Source/core/animation/Co...
Source/core/animation/CompositorAnimationsTest.cpp:264: static void
dispose(PassOwnPtrWillBeRawPtr<RenderObjectProxy> object)
On 2015/02/04 00:50:44, tkent wrote:
> Don't remove this function.  This is not specific to Oilpan, and we need to
call
> RenderObject::destroy().

Added back again (the use of destroy() was added as part of fixing this test for
Oilpan, btw.)

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
File Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:92: ALLOW_ONLY_INLINE_ALLOCATION();
On 2015/02/04 05:48:56, haraken wrote:
> 
> You can drop ALLOW_ONLY_INLINE_ALLOCATION().

Done, but does it add value to do so?

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:118: ALLOW_ONLY_INLINE_ALLOCATION();
On 2015/02/04 05:48:56, haraken wrote:
> 
> You can drop ALLOW_ONLY_INLINE_ALLOCATION().

Done.

https://codereview.chromium.org/869323003/diff/20001/Source/core/layout/Layou...
Source/core/layout/LayoutTableSection.h:323: #if ENABLE(OILPAN)
On 2015/02/04 00:50:44, tkent wrote:
> We can remove this block.
> 

Thanks, removed.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:61: static
OwnPtr<ContinuationMap>* continuationMap = 0;
On 2015/02/04 00:50:44, tkent wrote:
> This should be |ContinuationMap* continuationMap = nullptr|. We don't need to
> use OwnPtr.
> Also, (*continuationMap)->foo() should be continuationMap->foo().
> 

Reverted the use of OwnPtr<> & back to 10de152d4's representation of
continuationMap.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:61: static
OwnPtr<ContinuationMap>* continuationMap = 0;
On 2015/02/04 05:48:56, haraken wrote:
> On 2015/02/04 00:50:44, tkent wrote:
> > This should be |ContinuationMap* continuationMap = nullptr|. We don't need
to
> > use OwnPtr.
> > Also, (*continuationMap)->foo() should be continuationMap->foo().
> > 
> 
> Can we use DEFINE_STATIC_LOCAL?

Possibly, but lazy initialization appears to matter here, so I'd very much
prefer not to do co-tidying in this CL.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:669: ALLOW_ONLY_INLINE_ALLOCATION();
On 2015/02/04 05:48:56, haraken wrote:
> 
> You can drop ALLOW_ONLY_INLINE_ALLOCATION().

Done.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:250: ASSERT(m_didCallDestroy);
On 2015/02/04 05:48:56, haraken wrote:
> 
> Can we keep m_didCallDestroy? I think RenderObject::destroy() needs to be
called
> in both oilpan and non-oilpan even after this CL.

Why was it added&used for Oilpan only?

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderPart.cpp (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderPart.cpp:55: if (--m_refCount <= 0)
On 2015/02/04 05:48:56, haraken wrote:
> 
> Not related to this CL, it would be better to add:
> 
>   ASSERT(m_refCount >= 0);
> 
> and change the condition to if(!m_refCount).

That tidying seems preferable to handle in a followup.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTextFragment.h:85: // Oilpan: weak reference back to
GCed owner; its lifetime subsumes this object.
On 2015/02/04 05:48:56, haraken wrote:
> 
> I'm not sure of this comment. Even if RenderObject is on-heap, I guess
> m_firstLetterPseudoElement should be a strong reference (i.e., Member).

It was a Member<> when RenderObject was on heap, and cannot be now. It is
cleared on FirstLetterPseudoElement::detach().

Updated the comment wording.

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
Persistent<PatternAttributesWrapper> m_attributesWrapper;
On 2015/02/04 05:48:56, haraken wrote:
> 
> Can we avoid introducing a wrapper by making PatternAttributes not-a-part-of
> object in both oilpan and non-oilpan builds?

That would entail moving the value-based GradientAttributes hierarchy to the
heap and reworking the code that works over it to be imperative. Do we want to
do that & address that here?

[haraken, 2015-02-04 09:25:11]

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):

https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
Persistent<PatternAttributesWrapper> m_attributesWrapper;
On 2015/02/04 08:44:32, sof wrote:
> On 2015/02/04 05:48:56, haraken wrote:
> > 
> > Can we avoid introducing a wrapper by making PatternAttributes not-a-part-of
> > object in both oilpan and non-oilpan builds?
> 
> That would entail moving the value-based GradientAttributes hierarchy to the
> heap and reworking the code that works over it to be imperative. Do we want to
> do that & address that here?

I thought we can just use OwnPtrWillBePersistent<PatternAttributes> here, but
won't it work?

[sof, 2015-02-04 09:35:58]

On 2015/02/04 09:25:11, haraken wrote:
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):
> 
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
> Persistent<PatternAttributesWrapper> m_attributesWrapper;
> On 2015/02/04 08:44:32, sof wrote:
> > On 2015/02/04 05:48:56, haraken wrote:
> > > 
> > > Can we avoid introducing a wrapper by making PatternAttributes
not-a-part-of
> > > object in both oilpan and non-oilpan builds?
> > 
> > That would entail moving the value-based GradientAttributes hierarchy to the
> > heap and reworking the code that works over it to be imperative. Do we want
to
> > do that & address that here?
> 
> I thought we can just use OwnPtrWillBePersistent<PatternAttributes> here, but
> won't it work?

You can't have a Persistent to a part object, right?

[haraken, 2015-02-04 09:38:41]

On 2015/02/04 09:35:58, sof wrote:
> On 2015/02/04 09:25:11, haraken wrote:
> >
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):
> > 
> >
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> > Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
> > Persistent<PatternAttributesWrapper> m_attributesWrapper;
> > On 2015/02/04 08:44:32, sof wrote:
> > > On 2015/02/04 05:48:56, haraken wrote:
> > > > 
> > > > Can we avoid introducing a wrapper by making PatternAttributes
> not-a-part-of
> > > > object in both oilpan and non-oilpan builds?
> > > 
> > > That would entail moving the value-based GradientAttributes hierarchy to
the
> > > heap and reworking the code that works over it to be imperative. Do we
want
> to
> > > do that & address that here?
> > 
> > I thought we can just use OwnPtrWillBePersistent<PatternAttributes> here,
but
> > won't it work?
> 
> You can't have a Persistent to a part object, right?

ah, understood what you mean. We need to move a couple of hierarchies to the
heap. Let's do that in a separate CL.

LGTM.

[sof, 2015-02-04 09:44:40]

On 2015/02/04 09:38:41, haraken wrote:
> On 2015/02/04 09:35:58, sof wrote:
> > On 2015/02/04 09:25:11, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> > > File Source/core/rendering/svg/RenderSVGResourcePattern.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/869323003/diff/20001/Source/core/rendering/sv...
> > > Source/core/rendering/svg/RenderSVGResourcePattern.h:62:
> > > Persistent<PatternAttributesWrapper> m_attributesWrapper;
> > > On 2015/02/04 08:44:32, sof wrote:
> > > > On 2015/02/04 05:48:56, haraken wrote:
> > > > > 
> > > > > Can we avoid introducing a wrapper by making PatternAttributes
> > not-a-part-of
> > > > > object in both oilpan and non-oilpan builds?
> > > > 
> > > > That would entail moving the value-based GradientAttributes hierarchy to
> the
> > > > heap and reworking the code that works over it to be imperative. Do we
> want
> > to
> > > > do that & address that here?
> > > 
> > > I thought we can just use OwnPtrWillBePersistent<PatternAttributes> here,
> but
> > > won't it work?
> > 
> > You can't have a Persistent to a part object, right?
> 
> ah, understood what you mean. We need to move a couple of hierarchies to the
> heap. Let's do that in a separate CL.
> 
> LGTM.

thanks, let's do that. fwiw, i'm not enthused with these *Wrapper class
additions either.

[sof, 2015-02-04 09:49:50]

Thanks for the quick reviewing.

Last patchset has been given to the Oilpan bots. Should they report no issues &
there are no other issues raised here, will go ahead and land.

[sof, 2015-02-04 12:41:29]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-04 12:42:08]

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

[commit-bot: I haz the power, 2015-02-04 13:05:04]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189496

[haraken, 2015-02-04 14:03:28]

Thanks for landing this.

I'll re-collect performance numbers with ToT.

[haraken, 2015-02-04 14:03:53]

On 2015/02/04 14:03:28, haraken wrote:
> Thanks for landing this.
> 
> I'll re-collect performance numbers with ToT.

keishi-san@: Would you get the numbers on Mac? Sorry for asking a lot of
measurement :)