Bug 163414 – [Web IDL] Add support for [SameObject] extended attribute

• Status: REOPENED
• Component: Bindings

[Chris Dumez]

Add support for [SameObject] Web IDL extended attribute:
- https://heycam.github.io/webidl/#SameObject

[Chris Dumez]

Created attachment 291539 [details]
Patch

[Darin Adler]

Comment on attachment 291539 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:360
> +    return 1 if $attribute->signature->extendedAttributes->{CachedAttribute};
> +    return 1 if $attribute->signature->extendedAttributes->{SameObject};

Do these two attributes mean the same thing? Should there be two?

> LayoutTests/js/dom/SameObject-support-expected.txt:8
> +* document.implementation
> +PASS testObject[testAttributeName] === testObject[testAttributeName] is true
> +PASS testObject[testAttributeName].foo is 1

Would be more elegant if the test lines included "document" and "implementation" so we didn't have to explicitly label them above.

Might also be nice to test at least one case where [SameObject] is *not* specified.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 291539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291539&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:360
> > +    return 1 if $attribute->signature->extendedAttributes->{CachedAttribute};
> > +    return 1 if $attribute->signature->extendedAttributes->{SameObject};
> 
> Do these two attributes mean the same thing? Should there be two?

Yes, the 2 attributes currently do the same thing. However, I need to check remaining users of [CachedAttribute] to see if they really mean [SameObject] or not.

Most of the remaining uses of [CachedAttribute] also use [Custom] so I need to check what the custom code is doing. For example, [CachedAttribute] on History.state does not mean [SameObject]. The custom bindings code deals with invalidation of the cached attribute when the internal state changes. We may want to add an extended attribute to do this without custom bindings code (or re-use [CachedAttribute] for this purpose).

I'll clean things up a bit in follow-ups.

[Chris Dumez]

Created attachment 291554 [details]
Patch

[Chris Dumez]

Created attachment 291555 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 291555 [details]
Patch

Clearing flags on attachment: 291555

Committed r207319: <http://trac.webkit.org/changeset/207319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Hm, it seems that this causes performance regression in Dromaeo.
Could you take a look?

[Chris Dumez]

(In reply to comment #8)
> Hm, it seems that this causes performance regression in Dromaeo.
> Could you take a look?

Will do.

[Yusuke Suzuki]

(In reply to comment #9)
> (In reply to comment #8)
> > Hm, it seems that this causes performance regression in Dromaeo.
> > Could you take a look?
> 
> Will do.

Thanks!
Still I'm not 100% sure which revision occurs the regression... I'm investigating too...

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Hm, it seems that this causes performance regression in Dromaeo.
> > > Could you take a look?
> > 
> > Will do.
> 
> Thanks!
> Still I'm not 100% sure which revision occurs the regression... I'm
> investigating too...

Looks like it is this commit based on the graph. Looks like what regressed is don-modification and dom-traverse.

dom-traverse does cover Node.childNodes which I now cache. the dom-modify makes less sense because I did not alter dom modification API (e.g. createElement / appendChild). That said, I did make wrapper objects bigger due to the members needed for caching.

[Chris Dumez]

Actually, here are the dom-traverse results:
Test	        Before	After
firstChild	1158.1	1054.9
lastChild	975.8	875.5
nextSibling	1600.1	1353.1
previousSibling	1561.6	1346.5
childNodes	1597.5	1581.0

So it does not look like childNodes is the one that greatly regressed. The most regressed are actually nextSibling / previousSibling / lastChild. I think those are JIT'ed so they're likely super fast. Since the test traverses the dom tree, those accessors return a new wrapper every time. Therefore, my best bet is that I made the construction of DOM wrapper slower, likely due to their new data members that are used for caching. Still investigating.

[Chris Dumez]

(In reply to comment #12)
> Actually, here are the dom-traverse results:
> Test	        Before	After
> firstChild	1158.1	1054.9
> lastChild	975.8	875.5
> nextSibling	1600.1	1353.1
> previousSibling	1561.6	1346.5
> childNodes	1597.5	1581.0
> 
> So it does not look like childNodes is the one that greatly regressed. The
> most regressed are actually nextSibling / previousSibling / lastChild. I
> think those are JIT'ed so they're likely super fast. Since the test
> traverses the dom tree, those accessors return a new wrapper every time.
> Therefore, my best bet is that I made the construction of DOM wrapper
> slower, likely due to their new data members that are used for caching.
> Still investigating.

I am gathering profiles. I have also just thought about the fact that this could have slowed down garbage collection. When an interface has cached attributes (or [SameObject] attributes), we generate a visitChildren() function so we can register the  data members used for caching.

[Yusuke Suzuki]

(In reply to comment #13)
> (In reply to comment #12)
> > Actually, here are the dom-traverse results:
> > Test	        Before	After
> > firstChild	1158.1	1054.9
> > lastChild	975.8	875.5
> > nextSibling	1600.1	1353.1
> > previousSibling	1561.6	1346.5
> > childNodes	1597.5	1581.0
> > 
> > So it does not look like childNodes is the one that greatly regressed. The
> > most regressed are actually nextSibling / previousSibling / lastChild. I
> > think those are JIT'ed so they're likely super fast. Since the test
> > traverses the dom tree, those accessors return a new wrapper every time.
> > Therefore, my best bet is that I made the construction of DOM wrapper
> > slower, likely due to their new data members that are used for caching.
> > Still investigating.
> 
> I am gathering profiles. I have also just thought about the fact that this
> could have slowed down garbage collection. When an interface has cached
> attributes (or [SameObject] attributes), we generate a visitChildren()
> function so we can register the  data members used for caching.

Interesting.
How about caching the C++ results in something like C++ rare data structure?
Or what do you think of adding a private JS property on-demand?

Saam and Filip, do you have any idea?

[Chris Dumez]

Reverted r207319 for reason:

Regressed Dromaeo and may have caused crashes under GuardMalloc (rdar://problem/28780835)

Committed r207355: <http://trac.webkit.org/changeset/207355>