Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
158713002:
Move popular DOM attributes to prototype chains (Closed)
|
Created:
6 years, 10 months ago by haraken Modified:
5 years, 10 months ago CC:
aandrey+blink_chromium.org, adamk+blink_chromium.org, alph+blink_chromium.org, apavlov+blink_chromium.org, arv+blink, blink-reviews, caseq+blink_chromium.org, Inactive, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, eustas+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, paulirish+reviews_chromium.org, pfeldman+blink_chromium.org, sof, vsevik+blink_chromium.org, watchdog-blink-watchlist_google.com, yurys+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMove popular DOM attributes to prototype chains
Design document:
https://docs.google.com/a/google.com/document/d/1yeHTCHhulVIlrKyx9_gCguAhLfcefVOa9uxxfW2LVG0/edit
Intent-to-ship-and-implement in blink-dev:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/H0MGw0jkdn4/discussion
This CL can cause a slight regression in a couple of micro-benchmarks (e.g., Dromaeo/dom-traverse, Dromaeo/jsquery-traverse etc) but that is expected. See this spreadsheet for full performance results:
https://docs.google.com/a/google.com/spreadsheets/d/1Q6h2A11a2R4Q_MRuv_diyZCNzQBFExziXkV2MwChw48/edit#gid=622133744
BUG=43394
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190312
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Messages
Total messages: 64 (9 generated)
PTAL
lgtm
On 2014/02/10 12:23:35, jochen wrote: > lgtm Thanks. I forgot to add a test, so added.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/60001
The CQ bit was unchecked by haraken@chromium.org
Giving up landing this CL, because I noticed that the behavior bug (https://code.google.com/p/chromium/issues/detail?id=325880) is not yet fixed. I cannot click "Reply-to-all" in Gmail. Investigating.
Thanks for being persistent. https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... File LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html (right): https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:1: <!DOCTYPE> <!DOCTYPE html> What you wrote still puts the document in quirks mode. https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:10: shouldBeTrue('div.__proto__.__proto__.__proto__.hasOwnProperty("id")'); Next time, please inspect the whole property descriptor. We should assert the values of get, set, configurable and enumerable. https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:12: shouldThrow('obj = Object.create(div); obj.id'); A better test is to extract the getters and setters and call them var descr = Object.getOwnPropertyDescriptor(Element.prototype, 'id'); shouldThrow('descr.get.call({})'); also test that the setter works. var a = document.createElement('a'); shouldBeUndefined('descr.set.call(a, "abc")'); shouldBeEqualToString('descr.get.call(a)', 'abc');
Thanks arv, I'll apply your comments when landing. In order to identify the cause of the broken gmail, Dan proposed to move a lot of DOM attributes to prototype chains and see if any layout test fails. If we're lucky, it will give us a minimized test case for the behavior bug.
OK, I'm getting close. - If I add [ExposeJSAccessors] on Element.id, gmail gets broken. I cannot click "Reply-to-all". - If I add [ExposeJSAccessors] on Element.tagName and Element.outerHTML, fast/dom/custom/type-extensions.html starts failing. I'm investigating the latter one.
Do you know why gmail gets broken?
On 2014/02/13 05:59:56, pfeldman wrote: > Do you know why gmail gets broken? That's exactly what I'm investigating now :) We suspect it's a V8 bug. (1) Gmail is broken, but gmail is too big to diagnose. (2) Move a lot of DOM attributes to prototype chains, and see if any layout test fails. (3) I found that fast/dom/custom/type-extensions.html fails if I move Element.tagName and Element.outerHTML. (<---- I'm here.) (4) Try to infer the bug in the gmail.
> (3) I found that fast/dom/custom/type-extensions.html fails if I move > Element.tagName and Element.outerHTML. (<---- I'm here.) > (4) Try to infer the bug in the gmail. the failure in type-extensions is due to bad test expectations. gmail doesn't use it, so the bug must be somewhere else
On 2014/02/13 10:47:36, dcarney wrote: > > (3) I found that fast/dom/custom/type-extensions.html fails if I move > > Element.tagName and Element.outerHTML. (<---- I'm here.) > > (4) Try to infer the bug in the gmail. > > the failure in type-extensions is due to bad test expectations. gmail doesn't > use it, so the bug must be somewhere else I think the failure in type-extensions is real but it's due to a bug of CustomElements. CustomElements are manually tweaking prototype chains, so it's possible that we need to fix the logic when moving DOM attributes to prototype chains. However, I agree that the gmail issue is different from it. I verified that Gmail is not using CustomElements. The Gmail issue depends on whether we specify [ExposeJSAccessors] on Element.id or not. I'm still investigating but it's very hard to trace the auto-generated JavaScript of Gmail.
i can instrument v8 and blink enough to find out where the differences are sneaking in, but it's a big project, so I don't expect to find anything for a couple of days
On 2014/02/13 11:00:26, dcarney wrote:
> i can instrument v8 and blink enough to find out where the differences are
> sneaking in, but it's a big project, so I don't expect to find anything for a
> couple of days
Thanks Dan.
It's very easy to reproduce.
(1) Add [ExposeJSAccessors] to Element.id.
(2) Go to Gmail and click any pull-down menu. ("More", "Reply", etc)
I investigated the difference and found that:
- An id is correctly set on elements.
- When we move a mouse, mouse events are triggered. Then JavaScript extracts an
id of _some_ element X. The sequence of X's is different between the correct
Gmail and the broken Gmail.
hmm, it's still hard to diagnose gmail :) Dan: Are you working on the diagnosis? If necessary, I'm planning to ask a gmail team for help.
On 2014/02/13 15:36:33, haraken wrote: > hmm, it's still hard to diagnose gmail :) > > Dan: Are you working on the diagnosis? If necessary, I'm planning to ask a gmail > team for help. Here is a much reduced test case. http://closure-library.googlecode.com/git/closure/goog/demos/container.html If I set a breakpoint in cointainer.js:830 the bug goes away. It looks like the following line of code might generate incorrect optimized code: var id = childElem.id || (childElem.id = child.getId()); HTH
@arv: thanks so much for the reduced test case. You've saved me a ton of debugging time. I've brought it done to a 10 line test case and I'm working on figuring out what's happening right now.
this should be fixed by https://codereview.chromium.org/166653003/
On 2014/02/14 13:49:01, dcarney wrote: > this should be fixed by > > https://codereview.chromium.org/166653003/ Thanks for the very quick fix! Next week I'll work on fixing the custom element issue since it would be another issue.
Dan: I confirmed that the custom element issue is also fixed by your V8-side CL. So this CL is now ready to land. Has the V8 CL already rolled into Chromium? https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... File LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html (right): https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:1: <!DOCTYPE> On 2014/02/10 14:58:15, arv wrote: > <!DOCTYPE html> > > What you wrote still puts the document in quirks mode. Done. https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:10: shouldBeTrue('div.__proto__.__proto__.__proto__.hasOwnProperty("id")'); On 2014/02/10 14:58:15, arv wrote: > Next time, please inspect the whole property descriptor. We should assert the > values of get, set, configurable and enumerable. Done. https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:12: shouldThrow('obj = Object.create(div); obj.id'); On 2014/02/10 14:58:15, arv wrote: > A better test is to extract the getters and setters and call them > > var descr = Object.getOwnPropertyDescriptor(Element.prototype, 'id'); > shouldThrow('descr.get.call({})'); > > also test that the setter works. > > var a = document.createElement('a'); > shouldBeUndefined('descr.set.call(a, "abc")'); > shouldBeEqualToString('descr.get.call(a)', 'abc'); Done.
On 2014/02/17 11:26:29, haraken wrote: > Dan: I confirmed that the custom element issue is also fixed by your V8-side CL. > So this CL is now ready to land. Has the V8 CL already rolled into Chromium? V8 already branched, so you might want to hold off this CL until after blink branched. > > https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... > File LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html (right): > > https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... > LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:1: <!DOCTYPE> > On 2014/02/10 14:58:15, arv wrote: > > <!DOCTYPE html> > > > > What you wrote still puts the document in quirks mode. > > Done. > > https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... > LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:10: > shouldBeTrue('div.__proto__.__proto__.__proto__.hasOwnProperty("id")'); > On 2014/02/10 14:58:15, arv wrote: > > Next time, please inspect the whole property descriptor. We should assert the > > values of get, set, configurable and enumerable. > > Done. > > https://codereview.chromium.org/158713002/diff/60001/LayoutTests/fast/dom/dom... > LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:12: shouldThrow('obj > = Object.create(div); obj.id'); > On 2014/02/10 14:58:15, arv wrote: > > A better test is to extract the getters and setters and call them > > > > var descr = Object.getOwnPropertyDescriptor(Element.prototype, 'id'); > > shouldThrow('descr.get.call({})'); > > > > also test that the setter works. > > > > var a = document.createElement('a'); > > shouldBeUndefined('descr.set.call(a, "abc")'); > > shouldBeEqualToString('descr.get.call(a)', 'abc'); > > Done.
V8 is rolled. Looks like it's time to land this CL.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/370001
Message was sent while issue was closed.
Change committed as 167680
Message was sent while issue was closed.
Dan: Do you think it is now perfectly safe to reland this CL? (i.e., V8's r19380 is not likely to be reverted from the trunk, is it?)
Let me land this CL. I'll keep an eye on perf bots.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/370001
Message was sent while issue was closed.
Committed patchset #3 manually as r168383 (presubmit successful).
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/370001
Now that there should be no performance regression, I'm relanding this CL. I remeasured performance and updated a CL description.
Message was sent while issue was closed.
Change committed as 170941
I'm speculatively relanding this CL. See https://code.google.com/p/chromium/issues/detail?id=43394#c77 for the rationale.
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/370001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/dom/Element.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/dom/Element.idl
Hunk #1 FAILED at 51.
Hunk #2 FAILED at 91.
2 out of 2 hunks FAILED -- saving rejects to file
Source/core/dom/Element.idl.rej
Patch: Source/core/dom/Element.idl
Index: Source/core/dom/Element.idl
diff --git a/Source/core/dom/Element.idl b/Source/core/dom/Element.idl
index
00d9f2cff62e025ed786de5a748989f59f7b04ef..6abcccb3fc1fb5e6ff3f7b5c1d21f3a88d483623
100644
--- a/Source/core/dom/Element.idl
+++ b/Source/core/dom/Element.idl
@@ -51,7 +51,7 @@
[PerWorldBindings] readonly attribute CSSStyleDeclaration style;
// DOM4
- [Reflect, PerWorldBindings] attribute DOMString id;
+ [Reflect, PerWorldBindings, ExposeJSAccessors] attribute DOMString id;
[TreatReturnedNullStringAs=Null, PerWorldBindings] readonly attribute
DOMString namespaceURI;
[TreatReturnedNullStringAs=Null, TreatNullAs=NullString, PerWorldBindings,
RaisesException=Setter] attribute DOMString prefix;
[TreatReturnedNullStringAs=Null, PerWorldBindings] readonly attribute
DOMString localName;
@@ -91,7 +91,7 @@
// HTML 5
HTMLCollection getElementsByClassName(DOMString classNames);
- [TreatNullAs=NullString, CustomElementCallbacks, PerWorldBindings,
ActivityLogging=SetterForIsolatedWorlds, RaisesException=Setter] attribute
DOMString innerHTML;
+ [TreatNullAs=NullString, CustomElementCallbacks, PerWorldBindings,
ActivityLogging=SetterForIsolatedWorlds, RaisesException=Setter,
ExposeJSAccessors] attribute DOMString innerHTML;
[TreatNullAs=NullString, CustomElementCallbacks, RaisesException=Setter]
attribute DOMString outerHTML;
[RaisesException, CustomElementCallbacks, MeasureAs=InsertAdjacentElement]
Element insertAdjacentElement(DOMString where, Element element);
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/158713002/590001
Message was sent while issue was closed.
Change committed as 174142
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + yukishiino@chromium.or
Message was sent while issue was closed.
shiino-san: This is a CL that aims at moving DOM attributes used in Dromaeo.
https://codereview.chromium.org/158713002/diff/590001/LayoutTests/fast/dom/do... File LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html (right): https://codereview.chromium.org/158713002/diff/590001/LayoutTests/fast/dom/do... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:10: shouldBeTrue('div.__proto__.__proto__.__proto__.hasOwnProperty("id")'); Lets make these tests more explicit: assert.equal(HTMLDivElement.prototype, Object.getPrototypeOf(div)); function findHolder(object, propertyName) { if (!object) return null; if (Object.getOwnPropertyDescriptor(object, propertyName)) { return object; } return findHolder(Object.getPrototypeOf(object), propertyName); } assert.equal(Element.prototype, findHolder(div, 'id')); https://codereview.chromium.org/158713002/diff/590001/LayoutTests/fast/dom/do... LayoutTests/fast/dom/dom-attribute-on-prototype-chain.html:13: shouldBeTrue('desc.get instanceof Function'); instanceof is another non explicit test. In this case `typeof desc.get === 'function'` is more explicit. If you really want to test that the prototype chain of desc.get you can do: assert.equal(Function.prototype, Object.getPrototypeOf(desc.get));
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
New patchsets have been uploaded after l-g-t-m from jochen@chromium.org
shiino-san: PTAL
haraken@chromium.org changed reviewers: + yukishiino@chromium.org - yukishiino@chromium.or
shiino-san: Would you take a look at this?
lgtm
Sorry about the confusion. I just wanted to get this CL reviewed by you :)
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/158713002/710001
Message was sent while issue was closed.
Committed patchset #10 (id:710001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190312
Message was sent while issue was closed.
LGTM
Message was sent while issue was closed.
Hi, [I sent a previous message which seems to have disappeared, so I'll try again. Apologies if it turns up twice!] As hinted at by the email to perf sheriffs this CL did cause a (significant) regression across many tests. The graphs are here: https://chromeperf.appspot.com/group_report?bug_id=459653 The bisect picked up on a number of possible CLs but this patch seems to be the main potential culprit. The bisect results are given here: https://code.google.com/p/chromium/issues/detail?id=459653 Please take a look at this as the regressions are significant, many being greater than 20%. Thanks!
Message was sent while issue was closed.
On 2015/02/19 14:18:18, picksi wrote: > Hi, > > [I sent a previous message which seems to have disappeared, so I'll try again. > Apologies if it turns up twice!] > > As hinted at by the email to perf sheriffs this CL did cause a (significant) > regression across many tests. The graphs are here: > > https://chromeperf.appspot.com/group_report?bug_id=459653 > > The bisect picked up on a number of possible CLs but this patch seems to be the > main potential culprit. The bisect results are given here: > > https://code.google.com/p/chromium/issues/detail?id=459653 > > Please take a look at this as the regressions are significant, many being > greater than 20%. > > Thanks! Thanks for the report! I commented on https://code.google.com/p/chromium/issues/detail?id=43394#c117. I think these regressions are within expected ranges. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
