Bug 163414

Created attachment 291539 [details] Patch

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.

(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.

Created attachment 291554 [details] Patch

Created attachment 291555 [details] Patch

Comment on attachment 291555 [details] Patch Clearing flags on attachment: 291555 Committed r207319: <http://trac.webkit.org/changeset/207319>

All reviewed patches have been landed. Closing bug.

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

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

(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...

(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.

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.

(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.

(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?

Reverted r207319 for reason: Regressed Dromaeo and may have caused crashes under GuardMalloc (rdar://problem/28780835) Committed r207355: <http://trac.webkit.org/changeset/207355>