Bug 163414 - [Web IDL] Add support for [SameObject] extended attribute
Summary: [Web IDL] Add support for [SameObject] extended attribute
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://heycam.github.io/webidl/#Same...
Keywords: WebExposed
Depends on:
Blocks: 163429
  Show dependency treegraph
 
Reported: 2016-10-13 16:02 PDT by Chris Dumez
Modified: 2016-10-14 15:19 PDT (History)
9 users (show)

See Also:


Attachments
Patch (43.28 KB, patch)
2016-10-13 16:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (39.74 KB, patch)
2016-10-13 18:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (39.74 KB, patch)
2016-10-13 18:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-10-13 16:02:41 PDT
Add support for [SameObject] Web IDL extended attribute:
- https://heycam.github.io/webidl/#SameObject
Comment 1 Chris Dumez 2016-10-13 16:53:00 PDT
Created attachment 291539 [details]
Patch
Comment 2 Darin Adler 2016-10-13 17:51:54 PDT
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.
Comment 3 Chris Dumez 2016-10-13 18:23:17 PDT
(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.
Comment 4 Chris Dumez 2016-10-13 18:47:19 PDT
Created attachment 291554 [details]
Patch
Comment 5 Chris Dumez 2016-10-13 18:49:56 PDT
Created attachment 291555 [details]
Patch
Comment 6 WebKit Commit Bot 2016-10-13 19:52:45 PDT
Comment on attachment 291555 [details]
Patch

Clearing flags on attachment: 291555

Committed r207319: <http://trac.webkit.org/changeset/207319>
Comment 7 WebKit Commit Bot 2016-10-13 19:52:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Yusuke Suzuki 2016-10-14 11:04:18 PDT
Hm, it seems that this causes performance regression in Dromaeo.
Could you take a look?
Comment 9 Chris Dumez 2016-10-14 11:04:55 PDT
(In reply to comment #8)
> Hm, it seems that this causes performance regression in Dromaeo.
> Could you take a look?

Will do.
Comment 10 Yusuke Suzuki 2016-10-14 11:29:22 PDT
(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...
Comment 11 Chris Dumez 2016-10-14 14:11:51 PDT
(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.
Comment 12 Chris Dumez 2016-10-14 14:25:48 PDT
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.
Comment 13 Chris Dumez 2016-10-14 14:58:29 PDT
(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.
Comment 14 Yusuke Suzuki 2016-10-14 15:11:27 PDT
(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?
Comment 15 Chris Dumez 2016-10-14 15:19:16 PDT
Reverted r207319 for reason:

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

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