Oilpan: move DOM string collection objects to the Oilpan heap.

Oilpan: move DOM string collection objects to the Oilpan heap.

Turn DOMTokenList, DOMStringList, DOMStringMap and their derived objects
into garbage collected objects.

R=haraken@chromium.org,ager@chromium.org,erik.corry@gmail.com
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173652

[sof, 2014-04-29 07:23:38]

When there's some time, please take a look.

Open issues / rough edges:

 - ElementRareData is an in-between state here; Element
   delegates to its trace(), but it isn't a GC
   allocated object of its own. For two reasons:

     - NodeRareData (which it derives from) hasn't been
       moved to the heap yet.
     - Having the active animations live just a bit longer
       via their persistent seems key. At least until
       core/animation/ has been moved over completely.

   Suggestions for how to handle ElementRareData? (I'd
   prefer not to add more Persistents; they shouldn't
   be needed for the element class list nor the dataset.)

 - The clang GC plugin insist that trace() for a subclass
   upcalls one level up only. Which makes sense, but it
   makes for some not very interesting trace() call chains
   for (e.g.,) HTMLOutputElement. Is there an agreed-upon
   style for how to write out these trace()s: to be
   'inline'd, or is it the assumption that a compiler will
   be able to shorten them out?

[Erik Corry, 2014-04-29 07:55:48]

I wonder if it would not be easier to move the raredatas first, but one would
have to try it, to see.

The preferred style has been to have the trace methods in the .cpp file, which
is going to make it very hard for the compiler to inline them.  I think we may
have to move them to the .h file, but there are issues with include order.  For
now I think we just put them in the cpp files.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
File Source/core/dom/DatasetDOMStringMap.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
Source/core/dom/DatasetDOMStringMap.h:65: Element* m_element;
Should this not be a Member?

[haraken, 2014-04-29 08:00:01]

Just DBC:

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
File Source/core/dom/DatasetDOMStringMap.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
Source/core/dom/DatasetDOMStringMap.h:65: Element* m_element;
On 2014/04/29 07:55:49, Erik Corry wrote:
> Should this not be a Member?

We can't use a Member to the Node hierarchy until mads lands his next change
(i.e., make all directions traceable in the Node hierarchy).

[sof, 2014-04-29 08:06:38]

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
File Source/core/dom/DatasetDOMStringMap.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
Source/core/dom/DatasetDOMStringMap.h:65: Element* m_element;
On 2014/04/29 08:00:02, haraken wrote:
> On 2014/04/29 07:55:49, Erik Corry wrote:
> > Should this not be a Member?
> 
> We can't use a Member to the Node hierarchy until mads lands his next change
> (i.e., make all directions traceable in the Node hierarchy).

ok, I thought we already had those (cf. TextTrackList::m_owner)?

It was an oversight not to use RawPtrWillBeMember<> here for m_element; will
fix.

[Mads Ager (chromium), 2014-04-29 08:52:08]

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
File Source/core/dom/DatasetDOMStringMap.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
Source/core/dom/DatasetDOMStringMap.h:65: Element* m_element;
On 2014/04/29 08:06:38, sof wrote:
> On 2014/04/29 08:00:02, haraken wrote:
> > On 2014/04/29 07:55:49, Erik Corry wrote:
> > > Should this not be a Member?
> > 
> > We can't use a Member to the Node hierarchy until mads lands his next change
> > (i.e., make all directions traceable in the Node hierarchy).
> 
> ok, I thought we already had those (cf. TextTrackList::m_owner)?
> 
> It was an oversight not to use RawPtrWillBeMember<> here for m_element; will
> fix.

We can use a Member here and trace it. We *cannot* replace any RefPtrs with
Members at this point. Right now a Member does not keep the hierarchy alive.
That is still managed by ref counting. Using a Member here does not change ref
counts and therefore would work (but would not keep the tree alive, only that
one element). I'm working on making sure that Members keep the entire tree
alive.

So, turning raw pointers into Members is fine. However, it does not give the
correct life-time guarantees yet. Turning RefPtrs into Members will lead to
premature tear down of parts of the Node hierarchy and therefore cannot be done
at this point.

[Mads Ager (chromium), 2014-04-29 09:05:13]

LGTM with comments.

If you land this before moving ElementRareData, could you work on moving
Element/NodeRareData next so we get rid of the plugin exceptions.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/ContainerNode.h
File Source/core/dom/ContainerNode.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.h:192: virtual void trace(Visitor* visitor)
OVERRIDE { Node::trace(visitor); }
If the only thing this trace method does is call the super trace method I don't
think you need it? But it doesn't hurt. I'm adding a trace method OVERRIDE here
in my next change anyway.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
File Source/core/dom/DatasetDOMStringMap.cpp (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DatasetDOMSt...
Source/core/dom/DatasetDOMStringMap.cpp:222: visitor->trace(m_element);
Yeah, since you are tracing m_element, please put it in a Member. We should stay
away from tracing raw pointers when we can. If I see a raw pointer that is not
in a union I will assume that it is not traced.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/Element.cpp#...
Source/core/dom/Element.cpp:3275: if (hasRareData())
Please add a FIXME: Oilpan that this should be removed when rare data has been
moved to the heap.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/ElementRareD...
File Source/core/dom/ElementRareData.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/ElementRareD...
Source/core/dom/ElementRareData.h:134: GC_PLUGIN_IGNORE("340522")
We should work on this bug report. It might be nice to move RareData to the heap
first as Erik suggests?

https://codereview.chromium.org/258143002/diff/1/Source/core/html/HTMLElement.h
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.h:97: virtual void trace(Visitor* visitor) OVERRIDE
{ Element::trace(visitor); }
Do you need to add this, it doesn't change the tracing to have it. Does the
plugin complaint if it is not there?

https://codereview.chromium.org/258143002/diff/1/Source/core/html/HTMLFormCon...
File Source/core/html/HTMLFormControlElement.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/html/HTMLFormCon...
Source/core/html/HTMLFormControlElement.h:112: virtual void trace(Visitor*
visitor) OVERRIDE { LabelableElement::trace(visitor); }
Ditto?

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
File Source/core/html/LabelableElement.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
Source/core/html/LabelableElement.h:46: virtual void trace(Visitor* visitor)
OVERRIDE { HTMLElement::trace(visitor); }
You don't need this one I think. The explicit call of a super trace method will
do the right thing here. It will call the inherited one so you don't need this
dummy trace override.

[haraken, 2014-04-29 09:22:55]

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
File Source/core/html/LabelableElement.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
Source/core/html/LabelableElement.h:46: virtual void trace(Visitor* visitor)
OVERRIDE { HTMLElement::trace(visitor); }
On 2014/04/29 09:05:14, Mads Ager (chromium) wrote:
> You don't need this one I think. The explicit call of a super trace method
will
> do the right thing here. It will call the inherited one so you don't need this
> dummy trace override.

We'll need the trace method anyway in the near future, since there are a couple
of elements that inherit LabelableElement.

For the Node/CSS hierarchy, I'd prefer to just implement a perfect trace chain
instead of assessing what class does not need to have a trace method.

[sof, 2014-04-29 09:29:33]

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
File Source/core/html/LabelableElement.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
Source/core/html/LabelableElement.h:46: virtual void trace(Visitor* visitor)
OVERRIDE { HTMLElement::trace(visitor); }
On 2014/04/29 09:22:56, haraken wrote:
> On 2014/04/29 09:05:14, Mads Ager (chromium) wrote:
> > You don't need this one I think. The explicit call of a super trace method
> will
> > do the right thing here. It will call the inherited one so you don't need
this
> > dummy trace override.
> 
> We'll need the trace method anyway in the near future, since there are a
couple
> of elements that inherit LabelableElement.
> 
> For the Node/CSS hierarchy, I'd prefer to just implement a perfect trace
chain
> instead of assessing what class does not need to have a trace method.

(The plugin wanting such chaining was the direct reason for adding it now.)

[Mads Ager (chromium), 2014-04-29 09:30:52]

On 2014/04/29 09:22:55, haraken wrote:
>
https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
> File Source/core/html/LabelableElement.h (right):
> 
>
https://codereview.chromium.org/258143002/diff/1/Source/core/html/LabelableEl...
> Source/core/html/LabelableElement.h:46: virtual void trace(Visitor* visitor)
> OVERRIDE { HTMLElement::trace(visitor); }
> On 2014/04/29 09:05:14, Mads Ager (chromium) wrote:
> > You don't need this one I think. The explicit call of a super trace method
> will
> > do the right thing here. It will call the inherited one so you don't need
this
> > dummy trace override.
> 
> We'll need the trace method anyway in the near future, since there are a
couple
> of elements that inherit LabelableElement.
> 
> For the Node/CSS hierarchy, I'd prefer to just implement a perfect trace chain
> instead of assessing what class does not need to have a trace method.

The plugin will assess that for you. :-)

Most of the node/css hierarchy will need the trace methods in the end. For now,
I don't see any need to add the ones that only call the super class trace. The
plugin will tell you if you have Members but no trace method and (soon) if you
have Persistents that should have been traced instead.

[sof, 2014-04-29 14:07:20]

On 2014/04/29 09:05:13, Mads Ager (chromium) wrote:
> LGTM with comments.
> 
> If you land this before moving ElementRareData, could you work on moving
> Element/NodeRareData next so we get rid of the plugin exceptions.
> 

Yes, better do this in an orderly manner. I've started on the rare data now &
will delay getting this CL ready until that is in place.

[Mads Ager (chromium), 2014-04-29 14:47:45]

On 2014/04/29 14:07:20, sof wrote:
> On 2014/04/29 09:05:13, Mads Ager (chromium) wrote:
> > LGTM with comments.
> > 
> > If you land this before moving ElementRareData, could you work on moving
> > Element/NodeRareData next so we get rid of the plugin exceptions.
> > 
> 
> Yes, better do this in an orderly manner. I've started on the rare data now &
> will delay getting this CL ready until that is in place.

Sounds great. Thanks! As far as I recall, the rare data setup is a little funky.
It might be worth the time to have a look at how it was dealt with on the
branch. I remember that there were some minor complications but I do not
remember the details. :-)

[sof, 2014-04-30 10:16:37]

On 2014/04/29 14:47:45, Mads Ager (chromium) wrote:
> On 2014/04/29 14:07:20, sof wrote:
> > On 2014/04/29 09:05:13, Mads Ager (chromium) wrote:
> > > LGTM with comments.
> > > 
> > > If you land this before moving ElementRareData, could you work on moving
> > > Element/NodeRareData next so we get rid of the plugin exceptions.
> > > 
> > 
> > Yes, better do this in an orderly manner. I've started on the rare data now
&
> > will delay getting this CL ready until that is in place.
> 
> Sounds great. Thanks! As far as I recall, the rare data setup is a little
funky.
> It might be worth the time to have a look at how it was dealt with on the
> branch. I remember that there were some minor complications but I do not
> remember the details. :-)

Thanks, been looking at that in detail. Have something that leaks
elements/document (shadow DOM specific), but is otherwise seemingly sane.
Working on understanding what relationship brings that about.

[Mads Ager (chromium), 2014-04-30 10:22:33]

Have you seen the GC tracing code that Kouhei put in? It might be helpful
for you in tracing down what exactly is holding on to the shadow roots /
elements on a simple page.

If you put gc_tracing=1 in your GYP_DEFINES it will dump stats at the end
of GC.

If you uncomment the following line:

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

it will pring tracing as you go which can help you figure out what is
keeping your element/shadow root alive.


On Wed, Apr 30, 2014 at 12:16 PM, <sigbjornf@opera.com> wrote:

> On 2014/04/29 14:47:45, Mads Ager (chromium) wrote:
>
>> On 2014/04/29 14:07:20, sof wrote:
>> > On 2014/04/29 09:05:13, Mads Ager (chromium) wrote:
>> > > LGTM with comments.
>> > >
>> > > If you land this before moving ElementRareData, could you work on
>> moving
>> > > Element/NodeRareData next so we get rid of the plugin exceptions.
>> > >
>> >
>> > Yes, better do this in an orderly manner. I've started on the rare data
>> now
>>
> &
>
>> > will delay getting this CL ready until that is in place.
>>
>
>  Sounds great. Thanks! As far as I recall, the rare data setup is a little
>>
> funky.
>
>> It might be worth the time to have a look at how it was dealt with on the
>> branch. I remember that there were some minor complications but I do not
>> remember the details. :-)
>>
>
> Thanks, been looking at that in detail. Have something that leaks
> elements/document (shadow DOM specific), but is otherwise seemingly sane.
> Working on understanding what relationship brings that about.
>
> https://codereview.chromium.org/258143002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sof, 2014-04-30 14:38:55]

On 2014/04/30 10:22:33, Mads Ager (chromium) wrote:
> Have you seen the GC tracing code that Kouhei put in? It might be helpful
> for you in tracing down what exactly is holding on to the shadow roots /
> elements on a simple page.
> 
> If you put gc_tracing=1 in your GYP_DEFINES it will dump stats at the end
> of GC.
> 
> If you uncomment the following line:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> it will pring tracing as you go which can help you figure out what is
> keeping your element/shadow root alive.
> 

That definitely helps getting a better picture of what's going on; nice work.

(Will try to get a break for the next couple of days, so may not follow up right
away.)

[sof, 2014-05-08 09:15:17]

With {Node,Element}RareData changes in place, rebased. PTAL?

[Mads Ager (chromium), 2014-05-08 11:52:42]

LGTM

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.h:94: virtual void trace(Visitor*) OVERRIDE;
It looks like this one is only calling the super class trace method. We can
remove it.

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.h (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.h:112: virtual void trace(Visitor*)
OVERRIDE;
Ditto.

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBDatabase.cpp:187: RefPtrWillBeMember<DOMStringList>
objectStoreNames = DOMStringList::create();
RefPtrWillBeRawPtr

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBDatabase.cpp:349: RefPtrWillBeMember<DOMStringList>
storeNames = DOMStringList::create();
RawPtr

[haraken, 2014-05-08 11:59:58]

LGTM.

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DOMTokenList.h
File Source/core/dom/DOMTokenList.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:46: virtual ~DOMTokenList() { };

Nit: ';' is not needed.

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/ClassLi...
File Source/core/html/ClassList.h (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/ClassLi...
Source/core/html/ClassList.h:63: ClassList(Element*);

Add explicit.

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:507: void
HTMLFormControlElement::trace(Visitor* visitor)

tkent-san already implemented this when moving FormAssociatedElement to the
heap. I think you just need to rebase.

[sof, 2014-05-08 14:05:21]

Thanks for the reviews

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DOMTokenList.h
File Source/core/dom/DOMTokenList.h (right):

https://codereview.chromium.org/258143002/diff/1/Source/core/dom/DOMTokenList...
Source/core/dom/DOMTokenList.h:46: virtual ~DOMTokenList() { };
On 2014/05/08 11:59:59, haraken wrote:
> 
> Nit: ';' is not needed.

Gone.

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/ClassLi...
File Source/core/html/ClassList.h (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/ClassLi...
Source/core/html/ClassList.h:63: ClassList(Element*);
On 2014/05/08 11:59:59, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.h:94: virtual void trace(Visitor*) OVERRIDE;
On 2014/05/08 11:52:43, Mads Ager (chromium) wrote:
> It looks like this one is only calling the super class trace method. We can
> remove it.

Removed it. (Perhaps I just imagined it, but think I added it on behest of the
GC plugin originally.)

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/258143002/diff/20001/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:507: void
HTMLFormControlElement::trace(Visitor* visitor)
On 2014/05/08 11:59:59, haraken wrote:
> 
> tkent-san already implemented this when moving FormAssociatedElement to the
> heap. I think you just need to rebase.

Yes, removed this trace() when rebasing just now.

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBDatabase.cpp:349: RefPtrWillBeMember<DOMStringList>
storeNames = DOMStringList::create();
On 2014/05/08 11:52:43, Mads Ager (chromium) wrote:
> RawPtr

Oops, a M-/ mishap :)

[sof, 2014-05-08 14:05:58]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-05-08 14:06:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sigbjornf@opera.com/258143002/40001

[Mads Ager (chromium), 2014-05-08 14:22:06]

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBDatabase.cpp (right):

https://codereview.chromium.org/258143002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBDatabase.cpp:349: RefPtrWillBeMember<DOMStringList>
storeNames = DOMStringList::create();
On 2014/05/08 14:05:22, sof wrote:
> On 2014/05/08 11:52:43, Mads Ager (chromium) wrote:
> > RawPtr
> 
> Oops, a M-/ mishap :)

Emacs for the win! ;-)

[commit-bot: I haz the power, 2014-05-08 14:50:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/6949)

[commit-bot: I haz the power, 2014-05-08 15:59:13]

Change committed as 173652