Bug 50246 – [v8] put all the implicitly reachable style elements into single v8 group

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[anton muhin]

See http://code.google.com/p/chromium/issues/detail?id=64695

the problem is simple and alike to the problem of implicit references between nodes in document tree.  The solution is to group all the style elements which are reachable via DOM API into v8's object group.

I don't know details of style elements reachability, so I do the same thing we do for nodes: I group all the style elements which belong to the same tree as determined by the root element of the tree.

[anton muhin]

Created attachment 75165 [details]
Group all style elements in v8 object group

[anton muhin]

Created attachment 75174 [details]
[minor] proper diff

Proper diff---the previous one fail to apply

[WebKit Review Bot]

Attachment 75174 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/v8/V8DOMMap.h', u'WebCore/bindings/v8/V8GCController.cpp']" exit_code: 1
WebCore/bindings/v8/V8GCController.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/V8GCController.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/V8GCController.cpp:358:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/V8GCController.cpp:362:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[anton muhin]

Created attachment 75175 [details]
Fixing style issues

[WebKit Review Bot]

Attachment 75175 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/v8/V8DOMMap.h', u'WebCore/bindings/v8/V8GCController.cpp']" exit_code: 1
WebCore/bindings/v8/V8GCController.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[anton muhin]

Created attachment 75178 [details]
Next round of style fixes

[David Levin]

Does this also fix (or could it be extended slightly to fix)
  http://trac.webkit.org/changeset/72344
and
  http://trac.webkit.org/changeset/72590
?

Also, it seems like this should remove the corresponding lines in test_expectations:
BUG_LOISLO : fast/dom/StyleSheet/gc-rule-children-wrappers.html = TEXT
BUG_LEVIN : fast/dom/StyleSheet/gc-inline-style-cssvalues.html = TEXT
BUG_LEVIN : fast/dom/StyleSheet/gc-styleheet-wrapper.xhtml = TEXT
BUGCR64695 : fast/dom/StyleSheet/gc-parent-rule.html = TEXT
BUGCR64695 : fast/dom/StyleSheet/gc-parent-stylesheet.html = TEXT
BUGCR64695 : fast/dom/StyleSheet/gc-declaration-parent-rule.html = TEXT

Trivial nit: ObjectGrouperVisitor::startMap (endMap) need 4 space indenting.

cc'ed a few folks whom I believe know v8 well enough to review this effectively.

[David Levin]

Added someone else who may find it interesting (since they also added a line in test expectations that could be fixed by this).

[anton muhin]

Created attachment 75291 [details]
Another style nit

[anton muhin]

(In reply to comment #7)
> Does this also fix (or could it be extended slightly to fix)
>   http://trac.webkit.org/changeset/72344
> and
>   http://trac.webkit.org/changeset/72590
> ?
> 
> Also, it seems like this should remove the corresponding lines in test_expectations:
> BUG_LOISLO : fast/dom/StyleSheet/gc-rule-children-wrappers.html = TEXT
> BUG_LEVIN : fast/dom/StyleSheet/gc-inline-style-cssvalues.html = TEXT
> BUG_LEVIN : fast/dom/StyleSheet/gc-styleheet-wrapper.xhtml = TEXT
> BUGCR64695 : fast/dom/StyleSheet/gc-parent-rule.html = TEXT
> BUGCR64695 : fast/dom/StyleSheet/gc-parent-stylesheet.html = TEXT
> BUGCR64695 : fast/dom/StyleSheet/gc-declaration-parent-rule.html = TEXT

Chances are high.  It's not quite convenient for me to run X apps (like test_shell) this week as I am traveling.  I can do it myself next week and clean unnecessary suppressions.

I'll try to fix reminding problems too.


> 
> Trivial nit: ObjectGrouperVisitor::startMap (endMap) need 4 space indenting.

Thanks a lot, fixed.

> 
> cc'ed a few folks whom I believe know v8 well enough to review this effectively.

Ojan is going to run my patch through layout tests and see if it's good.

BTW, I'd esp. appreciate a look from people who are good in WebKit CSS model---we need to ensure that all implicitly style elements are put into v8's object group.

Thanks a lot for your help, David.

[Ojan Vafai]

I did run the test through layout tests. It fixed fast/dom/StyleSheet/gc-parent-rule.html and fast/dom/StyleSheet/gc-parent-stylesheet.html, but not fast/dom/StyleSheet/gc-declaration-parent-rule.html.

I'd prefer someone more familiar with V8 bindings review this though.

[Nate Chapin]

Comment on attachment 75291 [details]
Another style nit

View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review

I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it...

> WebCore/bindings/v8/V8GCController.cpp:330
> +        /* FIXME: Re-enabled this code to avoid GCing these wrappers!
> +                      Currently this depends on looking up the wrapper
> +                      during a GC, but we don't know which isolated world
> +                      we're in, so it's unclear which map to look in...

Are this FIXME and commented-out block still valid?  I'm just confused because it appears they didn't move with the surrounding code.

> WebCore/bindings/v8/V8GCController.cpp:357
> +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor {
> +public:
> +    ObjectGrouperVisitor()
> +    {
> +    }
>  

This name seems too generic for what this class is doing (or am I misunderstanding?).  Perhaps StyleBaseGrouperVisitor?

> WebCore/bindings/v8/V8GCController.cpp:378
> +        // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below.
> +        // FIXME: check if there are other StyleBase wrappers we should care of.
> +        if (!V8CSSStyleSheet::info.equals(typeInfo)
> +            && !V8CSSCharsetRule::info.equals(typeInfo)
> +            && !V8CSSFontFaceRule::info.equals(typeInfo)
> +            && !V8CSSStyleRule::info.equals(typeInfo)
> +            && !V8CSSImportRule::info.equals(typeInfo)
> +            && !V8CSSMediaRule::info.equals(typeInfo)) {

A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject.

> WebCore/bindings/v8/V8GCController.cpp:423
>      ObjectGrouperVisitor objectGrouperVisitor;
> -    visitDOMNodesInCurrentThread(&objectGrouperVisitor);
> -    objectGrouperVisitor.applyGrouping();
> +    visitDOMObjectsInCurrentThread(&objectGrouperVisitor);

This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above.  Is it?

[anton muhin]

Thanks a lot for comments, Nate!

(In reply to comment #12)
> (From update of attachment 75291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review
> 
> I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it...
> 
> > WebCore/bindings/v8/V8GCController.cpp:330
> > +        /* FIXME: Re-enabled this code to avoid GCing these wrappers!
> > +                      Currently this depends on looking up the wrapper
> > +                      during a GC, but we don't know which isolated world
> > +                      we're in, so it's unclear which map to look in...
> 
> Are this FIXME and commented-out block still valid?  I'm just confused because it appears they didn't move with the surrounding code.

Kind of.  The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so).

> > WebCore/bindings/v8/V8GCController.cpp:357
> > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor {
> > +public:
> > +    ObjectGrouperVisitor()
> > +    {
> > +    }
> >  
> 
> This name seems too generic for what this class is doing (or am I misunderstanding?).  Perhaps StyleBaseGrouperVisitor?

Let me explain the reason I named it this way, and if you disagree, I'll rename.  I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes).  Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node".  Maybe something referencing DOMObject would be better.

> > WebCore/bindings/v8/V8GCController.cpp:378
> > +        // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below.
> > +        // FIXME: check if there are other StyleBase wrappers we should care of.
> > +        if (!V8CSSStyleSheet::info.equals(typeInfo)
> > +            && !V8CSSCharsetRule::info.equals(typeInfo)
> > +            && !V8CSSFontFaceRule::info.equals(typeInfo)
> > +            && !V8CSSStyleRule::info.equals(typeInfo)
> > +            && !V8CSSImportRule::info.equals(typeInfo)
> > +            && !V8CSSMediaRule::info.equals(typeInfo)) {
> 
> A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject.

Thanks, I'd prefer to postpone it then.

> > WebCore/bindings/v8/V8GCController.cpp:423
> >      ObjectGrouperVisitor objectGrouperVisitor;
> > -    visitDOMNodesInCurrentThread(&objectGrouperVisitor);
> > -    objectGrouperVisitor.applyGrouping();
> > +    visitDOMObjectsInCurrentThread(&objectGrouperVisitor);
> 
> This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above.  Is it?

Sorry, I am not sure I understand you: we use different visitors here.  What am I missing?

[Nate Chapin]

(In reply to comment #13)
> Thanks a lot for comments, Nate!
> 
> (In reply to comment #12)
> > (From update of attachment 75291 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review
> > 
> > I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it...
> > 
> > > WebCore/bindings/v8/V8GCController.cpp:330
> > > +        /* FIXME: Re-enabled this code to avoid GCing these wrappers!
> > > +                      Currently this depends on looking up the wrapper
> > > +                      during a GC, but we don't know which isolated world
> > > +                      we're in, so it's unclear which map to look in...
> > 
> > Are this FIXME and commented-out block still valid?  I'm just confused because it appears they didn't move with the surrounding code.
> 
> Kind of.  The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so).

Ok, I just wanted to make sure it hadn't been forgotten.

> 
> > > WebCore/bindings/v8/V8GCController.cpp:357
> > > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor {
> > > +public:
> > > +    ObjectGrouperVisitor()
> > > +    {
> > > +    }
> > >  
> > 
> > This name seems too generic for what this class is doing (or am I misunderstanding?).  Perhaps StyleBaseGrouperVisitor?
> 
> Let me explain the reason I named it this way, and if you disagree, I'll rename.  I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes).  Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node".  Maybe something referencing DOMObject would be better.

That seems reasonable to me (I was assuming that we wouldn't be grouping other types of objects in this way, but that's a silly assumption).  Perhaps DOMObjectGrouperVisitor then?

> 
> > > WebCore/bindings/v8/V8GCController.cpp:378
> > > +        // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below.
> > > +        // FIXME: check if there are other StyleBase wrappers we should care of.
> > > +        if (!V8CSSStyleSheet::info.equals(typeInfo)
> > > +            && !V8CSSCharsetRule::info.equals(typeInfo)
> > > +            && !V8CSSFontFaceRule::info.equals(typeInfo)
> > > +            && !V8CSSStyleRule::info.equals(typeInfo)
> > > +            && !V8CSSImportRule::info.equals(typeInfo)
> > > +            && !V8CSSMediaRule::info.equals(typeInfo)) {
> > 
> > A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject.
> 
> Thanks, I'd prefer to postpone it then.
> 
> > > WebCore/bindings/v8/V8GCController.cpp:423
> > >      ObjectGrouperVisitor objectGrouperVisitor;
> > > -    visitDOMNodesInCurrentThread(&objectGrouperVisitor);
> > > -    objectGrouperVisitor.applyGrouping();
> > > +    visitDOMObjectsInCurrentThread(&objectGrouperVisitor);
> > 
> > This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above.  Is it?
> 
> Sorry, I am not sure I understand you: we use different visitors here.  What am I missing?

Ah, yes, I misread the code in the NDEBUG block, they are different visitor types. Sorry about that.

[anton muhin]

Created attachment 75531 [details]
Addressing Nate's comments

[anton muhin]

Thanks for another round, Nate.  I've ended up renaming to DOMObjectGrouperVisitor (yummy).  May you have another look, please?

(In reply to comment #14)
> (In reply to comment #13)
> > Thanks a lot for comments, Nate!
> > 
> > (In reply to comment #12)
> > > (From update of attachment 75291 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review
> > > 
> > > I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it...
> > > 
> > > > WebCore/bindings/v8/V8GCController.cpp:330
> > > > +        /* FIXME: Re-enabled this code to avoid GCing these wrappers!
> > > > +                      Currently this depends on looking up the wrapper
> > > > +                      during a GC, but we don't know which isolated world
> > > > +                      we're in, so it's unclear which map to look in...
> > > 
> > > Are this FIXME and commented-out block still valid?  I'm just confused because it appears they didn't move with the surrounding code.
> > 
> > Kind of.  The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so).
> 
> Ok, I just wanted to make sure it hadn't been forgotten.
> 
> > 
> > > > WebCore/bindings/v8/V8GCController.cpp:357
> > > > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor {
> > > > +public:
> > > > +    ObjectGrouperVisitor()
> > > > +    {
> > > > +    }
> > > >  
> > > 
> > > This name seems too generic for what this class is doing (or am I misunderstanding?).  Perhaps StyleBaseGrouperVisitor?
> > 
> > Let me explain the reason I named it this way, and if you disagree, I'll rename.  I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes).  Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node".  Maybe something referencing DOMObject would be better.
> 
> That seems reasonable to me (I was assuming that we wouldn't be grouping other types of objects in this way, but that's a silly assumption).  Perhaps DOMObjectGrouperVisitor then?
> 
> > 
> > > > WebCore/bindings/v8/V8GCController.cpp:378
> > > > +        // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below.
> > > > +        // FIXME: check if there are other StyleBase wrappers we should care of.
> > > > +        if (!V8CSSStyleSheet::info.equals(typeInfo)
> > > > +            && !V8CSSCharsetRule::info.equals(typeInfo)
> > > > +            && !V8CSSFontFaceRule::info.equals(typeInfo)
> > > > +            && !V8CSSStyleRule::info.equals(typeInfo)
> > > > +            && !V8CSSImportRule::info.equals(typeInfo)
> > > > +            && !V8CSSMediaRule::info.equals(typeInfo)) {
> > > 
> > > A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject.
> > 
> > Thanks, I'd prefer to postpone it then.
> > 
> > > > WebCore/bindings/v8/V8GCController.cpp:423
> > > >      ObjectGrouperVisitor objectGrouperVisitor;
> > > > -    visitDOMNodesInCurrentThread(&objectGrouperVisitor);
> > > > -    objectGrouperVisitor.applyGrouping();
> > > > +    visitDOMObjectsInCurrentThread(&objectGrouperVisitor);
> > > 
> > > This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above.  Is it?
> > 
> > Sorry, I am not sure I understand you: we use different visitors here.  What am I missing?
> 
> Ah, yes, I misread the code in the NDEBUG block, they are different visitor types. Sorry about that.

[anton muhin]

Created attachment 75534 [details]
WebKit hacking from Chromium checkout over ssh sucks

[Nate Chapin]

Comment on attachment 75534 [details]
WebKit hacking from Chromium checkout over ssh sucks

View in context: https://bugs.webkit.org/attachment.cgi?id=75534&action=review

> WebCore/ChangeLog:22
> +        (WebCore::ObjectGrouperVisitor::ObjectGrouperVisitor):
> +        (WebCore::ObjectGrouperVisitor::startMap):
> +        (WebCore::ObjectGrouperVisitor::endMap):
> +        (WebCore::ObjectGrouperVisitor::visitDOMWrapper):

Nit: this didn't get changed to DOMObjectGrouperVisitor.

> WebCore/bindings/v8/V8GCController.cpp:385
> +        StyleBase* styleBase = static_cast<StyleBase*>(object);
>  
> -            ASSERT(i == nextKeyIndex);
> +        // We put the whole tree of style elements into a single object group.
> +        // To achieve that we group elements by the roots of their trees.
> +        StyleBase* root = styleBase;

I wish there were a way for this code to be clear without needing styleBase as a separate variable (it's only use is to initialize root), but I don't immediately see what it is.

[anton muhin]

Created attachment 75804 [details]
Last minor fixes

[anton muhin]

Thanks a lot, Nate.  Fixing and cq+'ing

(In reply to comment #18)
> (From update of attachment 75534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75534&action=review
> 
> > WebCore/ChangeLog:22
> > +        (WebCore::ObjectGrouperVisitor::ObjectGrouperVisitor):
> > +        (WebCore::ObjectGrouperVisitor::startMap):
> > +        (WebCore::ObjectGrouperVisitor::endMap):
> > +        (WebCore::ObjectGrouperVisitor::visitDOMWrapper):
> 
> Nit: this didn't get changed to DOMObjectGrouperVisitor.
> 
> > WebCore/bindings/v8/V8GCController.cpp:385
> > +        StyleBase* styleBase = static_cast<StyleBase*>(object);
> >  
> > -            ASSERT(i == nextKeyIndex);
> > +        // We put the whole tree of style elements into a single object group.
> > +        // To achieve that we group elements by the roots of their trees.
> > +        StyleBase* root = styleBase;
> 
> I wish there were a way for this code to be clear without needing styleBase as a separate variable (it's only use is to initialize root), but I don't immediately see what it is.

[anton muhin]

Created attachment 75805 [details]
With proper reviewed by

[WebKit Commit Bot]

Comment on attachment 75805 [details]
With proper reviewed by

Clearing flags on attachment: 75805

Committed r73491: <http://trac.webkit.org/changeset/73491>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.