Oilpan: move SVG property hierarchy to the heap.

Oilpan: move SVG property hierarchy to the heap.

Turn SVGPropertyBase into a non-refcounted, GCed object
and move all its derived objects to the heap.

R=kouhei,haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185240

[sof, 2014-10-30 07:19:19]

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

[sof, 2014-10-30 07:19:23]

Please take a look.

As agreed, let's move ahead with this one (as time permits.)

[kouhei (in TOK), 2014-10-30 08:06:53]

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

[kouhei (in TOK), 2014-10-30 08:06:55]

core/svg/properties lgtm. core/svg lgtm assuming only ptr changes.

[haraken, 2014-11-11 05:22:42]

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

[haraken, 2014-11-11 05:22:47]

Thanks for working on this!

This CL looks mostly straightforward. LGTM.

https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
File Source/core/animation/animatable/AnimatableSVGLength.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
Source/core/animation/animatable/AnimatableSVGLength.h:59:
AnimatableSVGLength(PassRefPtrWillBeRawPtr<SVGLength> length)

Add explicit.

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:1645: #if ENABLE(OILPAN)

Can we avoid introducing the #if flag?

DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<>, ...)

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/StyleFilterData.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/StyleFilterData.h:36: class StyleFilterData final :
public RefCountedWillBeRefCountedGarbageCollected<StyleFilterData> {

Why does this need to be RefCountedGarbageCollected?

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.h:95: typedef
WillBeHeapHashMap<RenderObject*, OwnPtrWillBeMember<FilterData> > FilterMap;

RenderObject* => RawPtrWillBeMember<RenderObject> ?

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

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourcePattern.h:71: HashMap<const
RenderObject*, OwnPtr<PatternData> > m_patternMap;

Not related to this CL, this could be HeapHashMap<Member<RenderObject>, ...>.
You don't need to address it in this CL.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
File Source/core/svg/PatternAttributes.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
Source/core/svg/PatternAttributes.h:32: struct PatternAttributes final {

struct => class

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
Source/core/svg/PatternAttributes.h:160: const SVGPatternElement*
m_patternContentElement;

This should be a Member.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimatedAngle.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimatedAngle.h:63: SVGAnimatedAngle(SVGMarkerElement*
contextElement);

Add explicit.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumber.h
File Source/core/svg/SVGNumber.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumbe...
Source/core/svg/SVGNumber.h:97: SVGNumberAcceptPercentage(float value);

Add explicit.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSeg.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSeg.h:114: m_ownerList = ownerList;

It looks like this is called in some destructors, but are they OK? (I haven't
looked at the code.)

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSeg.h:119: m_contextElement = contextElement;

Ditto.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSegList.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSegList.h:152: SVGPathSegList(SVGPathElement*);

Add explicit.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathStringSource.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathStringSource.h:38: SVGPathStringSource(const String&);

Add explicit.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
File Source/core/svg/properties/SVGProperty.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
Source/core/svg/properties/SVGProperty.h:83: void setOwnerList(SVGPropertyBase*
ownerList)

Just to confirm: Isn't it possible that setOwnerList(0) is called in some
destructors?

https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/filters/FilterOperations.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
Source/platform/graphics/filters/FilterOperations.h:89: class
FilterOperationsWrapper : public GarbageCollected<FilterOperationsWrapper> {

Would you help me understand why we need special wrapper objects for oilpan?

[sof, 2014-11-12 13:45:08]

Thanks for the reviews.

Reconfirming that this doesn't regress Oilpan performance on blink_perf.svg
before taking the next step.

https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
File Source/core/animation/animatable/AnimatableSVGLength.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
Source/core/animation/animatable/AnimatableSVGLength.h:59:
AnimatableSVGLength(PassRefPtrWillBeRawPtr<SVGLength> length)
On 2014/11/11 05:22:46, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:1645: #if ENABLE(OILPAN)
On 2014/11/11 05:22:46, haraken wrote:
> 
> Can we avoid introducing the #if flag?
> 
> DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<>, ...)

I don't think CPP is entirely avoidable, as we need to use a separate type for
Oilpan (FilterOperationsWrapper vs FilterOperations), so I think it makes sense
to have it like this.

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
File Source/core/rendering/style/StyleFilterData.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
Source/core/rendering/style/StyleFilterData.h:36: class StyleFilterData final :
public RefCountedWillBeRefCountedGarbageCollected<StyleFilterData> {
On 2014/11/11 05:22:46, haraken wrote:
> 
> Why does this need to be RefCountedGarbageCollected?

StyleRareNonInheritedData has a DataRef<StyleFilterData>. I don't know of a
workable Oilpan, non-RefPtr<>, alternative for DataRef<>.

I've added a FIXME to make it locally clear why we're using such a type.

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.h:95: typedef
WillBeHeapHashMap<RenderObject*, OwnPtrWillBeMember<FilterData> > FilterMap;
On 2014/11/11 05:22:46, haraken wrote:
> 
> RenderObject* => RawPtrWillBeMember<RenderObject> ?

Done, a safe change wrt lifetime. Also tidied up the stack allocated
SVGRenderingContext (which refers to RenderSVGResourceFilter) while at it.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
File Source/core/svg/PatternAttributes.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
Source/core/svg/PatternAttributes.h:32: struct PatternAttributes final {
On 2014/11/11 05:22:46, haraken wrote:
> 
> struct => class

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
Source/core/svg/PatternAttributes.h:160: const SVGPatternElement*
m_patternContentElement;
On 2014/11/11 05:22:46, haraken wrote:
> 
> This should be a Member.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
File Source/core/svg/SVGAnimatedAngle.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
Source/core/svg/SVGAnimatedAngle.h:63: SVGAnimatedAngle(SVGMarkerElement*
contextElement);
On 2014/11/11 05:22:46, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumber.h
File Source/core/svg/SVGNumber.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumbe...
Source/core/svg/SVGNumber.h:97: SVGNumberAcceptPercentage(float value);
On 2014/11/11 05:22:46, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSeg.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSeg.h:114: m_ownerList = ownerList;
On 2014/11/11 05:22:46, haraken wrote:
> 
> It looks like this is called in some destructors, but are they OK? (I haven't
> looked at the code.)

It is not called from destructors (double checking now). It is called by
SVGListPropertyHelper<..>::clear(), but clear() is only invoked from a dtor if
!OILPAN.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSeg.h:119: m_contextElement = contextElement;
On 2014/11/11 05:22:46, haraken wrote:
> 
> Ditto.

Same, never called to clear out m_contextElement.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathSegList.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathSegList.h:152: SVGPathSegList(SVGPathElement*);
On 2014/11/11 05:22:46, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
File Source/core/svg/SVGPathStringSource.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
Source/core/svg/SVGPathStringSource.h:38: SVGPathStringSource(const String&);
On 2014/11/11 05:22:46, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
File Source/core/svg/properties/SVGProperty.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
Source/core/svg/properties/SVGProperty.h:83: void setOwnerList(SVGPropertyBase*
ownerList)
On 2014/11/11 05:22:46, haraken wrote:
> 
> Just to confirm: Isn't it possible that setOwnerList(0) is called in some
> destructors?

It doesn't currently happen. We can't statically catch anyone adding such a call
from a SVGProperty list implementation in the future, but I would expect ASAN
and tests to pick up on that pretty quickly.

https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
File Source/platform/graphics/filters/FilterOperations.h (right):

https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
Source/platform/graphics/filters/FilterOperations.h:89: class
FilterOperationsWrapper : public GarbageCollected<FilterOperationsWrapper> {
On 2014/11/11 05:22:47, haraken wrote:
> 
> Would you help me understand why we need special wrapper objects for oilpan?

We want to have FilterOperations be a part/value object, but for one use and
persistent reference from RenderStyle::initialFilter().

[sof, 2014-11-12 15:49:52]

On 2014/11/12 13:45:08, sof wrote:
> Thanks for the reviews.
> 
> Reconfirming that this doesn't regress Oilpan performance on blink_perf.svg
> before taking the next step.
> 

https://docs.google.com/spreadsheets/d/1ws14v8fW8jMY0IgociGhejaTGWofQxobjn2lM...
has the numbers.

Performance is the same, arguably slightly better on a couple of blink_perf.svg
tests.

[sof, 2014-11-12 19:13:00]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-11-12 19:13:36]

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

[commit-bot: I haz the power, 2014-11-12 20:03:23]

Committed patchset #7 (id:120001) as 185240

[haraken, 2014-11-13 02:10:42]

On 2014/11/12 13:45:08, sof wrote:
> Thanks for the reviews.
> 
> Reconfirming that this doesn't regress Oilpan performance on blink_perf.svg
> before taking the next step.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
> File Source/core/animation/animatable/AnimatableSVGLength.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/animation/an...
> Source/core/animation/animatable/AnimatableSVGLength.h:59:
> AnimatableSVGLength(PassRefPtrWillBeRawPtr<SVGLength> length)
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Add explicit.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
> File Source/core/rendering/style/RenderStyle.cpp (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/RenderStyle.cpp:1645: #if ENABLE(OILPAN)
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Can we avoid introducing the #if flag?
> > 
> > DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<>, ...)
> 
> I don't think CPP is entirely avoidable, as we need to use a separate type for
> Oilpan (FilterOperationsWrapper vs FilterOperations), so I think it makes
sense
> to have it like this.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
> File Source/core/rendering/style/StyleFilterData.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/st...
> Source/core/rendering/style/StyleFilterData.h:36: class StyleFilterData final
:
> public RefCountedWillBeRefCountedGarbageCollected<StyleFilterData> {
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Why does this need to be RefCountedGarbageCollected?
> 
> StyleRareNonInheritedData has a DataRef<StyleFilterData>. I don't know of a
> workable Oilpan, non-RefPtr<>, alternative for DataRef<>.
> 
> I've added a FIXME to make it locally clear why we're using such a type.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGResourceFilter.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGResourceFilter.h:95: typedef
> WillBeHeapHashMap<RenderObject*, OwnPtrWillBeMember<FilterData> > FilterMap;
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > RenderObject* => RawPtrWillBeMember<RenderObject> ?
> 
> Done, a safe change wrt lifetime. Also tidied up the stack allocated
> SVGRenderingContext (which refers to RenderSVGResourceFilter) while at it.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
> File Source/core/svg/PatternAttributes.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
> Source/core/svg/PatternAttributes.h:32: struct PatternAttributes final {
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > struct => class
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/PatternA...
> Source/core/svg/PatternAttributes.h:160: const SVGPatternElement*
> m_patternContentElement;
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > This should be a Member.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
> File Source/core/svg/SVGAnimatedAngle.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGAnima...
> Source/core/svg/SVGAnimatedAngle.h:63: SVGAnimatedAngle(SVGMarkerElement*
> contextElement);
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Add explicit.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumber.h
> File Source/core/svg/SVGNumber.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGNumbe...
> Source/core/svg/SVGNumber.h:97: SVGNumberAcceptPercentage(float value);
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Add explicit.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> File Source/core/svg/SVGPathSeg.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> Source/core/svg/SVGPathSeg.h:114: m_ownerList = ownerList;
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > It looks like this is called in some destructors, but are they OK? (I
haven't
> > looked at the code.)
> 
> It is not called from destructors (double checking now). It is called by
> SVGListPropertyHelper<..>::clear(), but clear() is only invoked from a dtor if
> !OILPAN.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> Source/core/svg/SVGPathSeg.h:119: m_contextElement = contextElement;
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Ditto.
> 
> Same, never called to clear out m_contextElement.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> File Source/core/svg/SVGPathSegList.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> Source/core/svg/SVGPathSegList.h:152: SVGPathSegList(SVGPathElement*);
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Add explicit.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> File Source/core/svg/SVGPathStringSource.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/SVGPathS...
> Source/core/svg/SVGPathStringSource.h:38: SVGPathStringSource(const String&);
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Add explicit.
> 
> Done.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
> File Source/core/svg/properties/SVGProperty.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/core/svg/properti...
> Source/core/svg/properties/SVGProperty.h:83: void
setOwnerList(SVGPropertyBase*
> ownerList)
> On 2014/11/11 05:22:46, haraken wrote:
> > 
> > Just to confirm: Isn't it possible that setOwnerList(0) is called in some
> > destructors?
> 
> It doesn't currently happen. We can't statically catch anyone adding such a
call
> from a SVGProperty list implementation in the future, but I would expect ASAN
> and tests to pick up on that pretty quickly.
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
> File Source/platform/graphics/filters/FilterOperations.h (right):
> 
>
https://codereview.chromium.org/678163002/diff/80001/Source/platform/graphics...
> Source/platform/graphics/filters/FilterOperations.h:89: class
> FilterOperationsWrapper : public GarbageCollected<FilterOperationsWrapper> {
> On 2014/11/11 05:22:47, haraken wrote:
> > 
> > Would you help me understand why we need special wrapper objects for oilpan?
> 
> We want to have FilterOperations be a part/value object, but for one use and
> persistent reference from RenderStyle::initialFilter().

Thanks, all your replies make sense :)