Bug 75297

Summary:

[V8] meta: DOM attributes should be defined on a prototype chain

Product:

WebKit

Reporter:

Kentaro Hara <haraken>

Component:

New Bugs

Assignee:

Kentaro Hara <haraken>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

abarth, andersca, ap, arv, dglazkov, dominicc, japhet, ojan, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

49739

Attachments:

Description	Flags
Test cases for constants, attributes and operations	none
Test cases for attribute getter and setter	none
WIP: test cases	none
WIP: test cases	none
WIP patch	none
DOM micro benchmark	none
DOM micro benchmark	none
WIP: The OnProto patch	none
WIP: Patch to measure the overhead of DOM attributes	none
Patch for performance experiments. Not for review.	none
Benchmark tests for performance experiments	none

Kentaro Hara

Reported 2011-12-28 00:54:47 PST

Although currently DOM attributes are defined on a platform object, the spec says that DOM attributes without [Unforgeable] IDL should be defined on a prototype chain: http://dev.w3.org/2006/webapi/WebIDL/#es-attributes Current behavior: HTMLElement.hasOwnProperty("lang") => false HTMLElement.prototype.hasOwnProperty("lang") => false div.hasOwnProperty("lang") => true div.__proto__.hasOwnProperty("lang") => false div.__proto__.__proto__.hasOwnProperty("lang") => false Expected behavior: HTMLElement.hasOwnProperty("lang") => false HTMLElement.prototype.hasOwnProperty("lang") => true div.hasOwnProperty("lang") => false div.__proto__.hasOwnProperty("lang") => false div.__proto__.__proto__.hasOwnProperty("lang") => true

Attachments
Test cases for constants, attributes and operations (3.03 KB, text/html) 2011-12-28 00:56 PST, Kentaro Hara	no flags	Details
Test cases for attribute getter and setter (3.29 KB, text/html) 2011-12-28 00:58 PST, Kentaro Hara	no flags	Details
WIP: test cases (7.93 KB, patch) 2011-12-28 01:03 PST, Kentaro Hara	no flags	Details Formatted Diff Diff
WIP: test cases (7.80 KB, patch) 2011-12-28 06:02 PST, Kentaro Hara	no flags	Details Formatted Diff Diff
WIP patch (62.12 KB, patch) 2012-01-03 08:37 PST, Kentaro Hara	no flags	Details Formatted Diff Diff
DOM micro benchmark (5.23 KB, text/html) 2012-01-05 22:20 PST, Kentaro Hara	no flags	Details
DOM micro benchmark (5.35 KB, text/html) 2012-01-05 22:29 PST, Kentaro Hara	no flags	Details
WIP: The OnProto patch (62.67 KB, patch) 2012-01-18 17:52 PST, Kentaro Hara	no flags	Details Formatted Diff Diff
WIP: Patch to measure the overhead of DOM attributes (70.85 KB, patch) 2012-01-18 18:10 PST, Kentaro Hara	no flags	Details Formatted Diff Diff
Patch for performance experiments. Not for review. (24.18 KB, patch) 2012-10-02 01:05 PDT, Kentaro Hara	no flags	Details Formatted Diff Diff
Benchmark tests for performance experiments (9.71 KB, text/html) 2012-10-02 01:08 PDT, Kentaro Hara	no flags	Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Kentaro Hara

Comment 1 2011-12-28 00:56:37 PST

Created attachment 120638 [details] Test cases for constants, attributes and operations This test checks if constants, attributes and operations on a DOM interface are correctly implemented.

Kentaro Hara

Comment 2 2011-12-28 00:58:18 PST

Created attachment 120639 [details] Test cases for attribute getter and setter This test checks if attribute getter and setter on a DOM interface are correctly implemented.

Kentaro Hara

Comment 3 2011-12-28 01:03:13 PST

Created attachment 120641 [details] WIP: test cases

Kentaro Hara

Comment 4 2011-12-28 06:02:50 PST

Created attachment 120661 [details] WIP: test cases

Alexey Proskuryakov

Comment 5 2011-12-28 19:44:57 PST

Don't we have bugs about this already? Perhaps bug 68002 and/or bug 49739.

Kentaro Hara

Comment 6 2011-12-29 06:44:02 PST

(In reply to comment #5) > Don't we have bugs about this already? Perhaps bug 68002 and/or bug 49739. ap: Yes, this is a duplicated bug. We (i.e. dominicc and I) are planning to use this bug just for informally investigating WIP strategies for addressing this issue. After some investigation, we will upload a patch for the original bug 49739. Thanks. *** This bug has been marked as a duplicate of bug 49739 ***

Kentaro Hara

Comment 7 2012-01-03 08:36:32 PST

ap: On this bug, I would like to work on the V8 side fix of bug 49739.

Kentaro Hara

Comment 8 2012-01-03 08:37:23 PST

Created attachment 120952 [details] WIP patch

Kentaro Hara

Comment 9 2012-01-03 08:43:51 PST

I uploaded a WIP (or EXPERIMENTAL) patch. I moved all the V8 DOM attributes in HTMLDivElement.idl, HTMLElement.idl, Element.idl and Node.idl (except for the attributes that have custom getters or custom setters), from a platform object to a prototype chain. Please see WebCore/ChangeLog for more details. Note: This is just a WIP patch. Ultimately, we are planning to remove all the [V8OnProto] IDL and just use the [V8Unforgeable] IDL; In other words, according to the spec, all attributes without the [V8Unforgeable] IDL will be defined on a prototype chain and all attributes with the [V8Unforgeable] IDL will be defined on a platform object. That being said, this requires non-trivial work. Before moving all DOM attributes onto a prototype chain, we need to - rewrite all custom getters and setters, - handle special attributes (e.g. DOMWindow attributes, EventListener attributes, attributes with the [V8DisallowShadowing] IDL) - investigate security issues, - and measure the performance impact of this change. Comments are appreciated. In particular, I am wondering if the test cases (interface_attributes.html and interface_properties.html) are sufficient and correct.

Dominic Cooney

Comment 10 2012-01-04 21:57:35 PST

Comment on attachment 120952 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=120952&action=review > Source/WebCore/ChangeLog:28 > + if (!V8HTMLElement::GetTemplate()->HasInstance(info.This())) /* (#) */ What happens with: HTMLButtonElement.prototype.lang > Source/WebCore/ChangeLog:69 > + * bindings/scripts/CodeGeneratorV8.pm: As described above. Renamed [v8OnProto] For clarity it might be good to separate the rename out from the rest of this change. > LayoutTests/fast/js/interface_attributes.html:1 > +<!DOCTYPE html> These tests look good to me. > LayoutTests/fast/js/interface_attributes.html:14 > +shouldBe('div.__proto__ === HTMLDivElement.prototype', 'true'); Is there shouldBeTrue? > LayoutTests/fast/js/interface_properties.html:1 > +<!DOCTYPE html> These tests look good to me.

Kentaro Hara

Comment 11 2012-01-04 22:21:20 PST

Comment on attachment 120952 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=120952&action=review >> Source/WebCore/ChangeLog:28 >> + if (!V8HTMLElement::GetTemplate()->HasInstance(info.This())) /* (#) */ > > What happens with: > > HTMLButtonElement.prototype.lang Both HTMLButtonElement.prototype.lang and HTMLDivElement.prototype.lang throw TypeError (I confirmed). Since HTML{Button,Div}Element.prototype is not a platform-object, HasInstance() returns false. Although more security and performance investigation is needed, if this change looks OK, we might be able to get this work done without touching the V8 code at all. > Source/WebCore/ChangeLog:62 > + attributes with the [V8DisallowShadowing] IDL) Actually I am not sure what attributes must be treated specially. It seems that the attributes with the [Unforgeable] IDL in the spec are not the only attributes we must treat specially. I'm investigating it.

Kentaro Hara

Comment 12 2012-01-05 06:36:14 PST

I evaluated the performance impact using DROMAEO. Since the patch moved all DOM attributes in HTMLElement, Element and Node, I think that we can approximately evaluate the performance impact. The current performance (DOM attributes on a platform object): http://dromaeo.com/?id=159876 The performance with the patch (DOM attributes on a prototype chain): http://dromaeo.com/?id=159879 For example, the performance of "DOM attributes" degrades by 24%. Oh...this is far from acceptable...

Dimitri Glazkov (Google)

Comment 13 2012-01-05 09:08:39 PST

(In reply to comment #12) > I evaluated the performance impact using DROMAEO. Since the patch moved all DOM attributes in HTMLElement, Element and Node, I think that we can approximately evaluate the performance impact. > > The current performance (DOM attributes on a platform object): http://dromaeo.com/?id=159876 > The performance with the patch (DOM attributes on a prototype chain): http://dromaeo.com/?id=159879 > > For example, the performance of "DOM attributes" degrades by 24%. Oh...this is far from acceptable... Yikes.

Kentaro Hara

Comment 14 2012-01-05 22:20:24 PST

Created attachment 121399 [details] DOM micro benchmark Uploaded a DOM micro benchmark. The result will be coming soon...

Kentaro Hara

Comment 15 2012-01-05 22:29:40 PST

Created attachment 121401 [details] DOM micro benchmark Updated the DOM micro benchmark.

Kentaro Hara

Comment 16 2012-01-05 23:01:47 PST

Here are the performance results of the DOM micro benchmark I attached: The current performance (DOM attributes on a platform object): http://haraken.info/a/dom_benchmark_master.txt The performance with the patch (DOM attributes on a prototype chain): http://haraken.info/a/dom_benchmark_wip.txt In total, moving DOM attributes degrades performance by 12%.

Kentaro Hara

Comment 17 2012-01-18 17:52:17 PST

Created attachment 123046 [details] WIP: The OnProto patch

Kentaro Hara

Comment 18 2012-01-18 18:10:15 PST

Created attachment 123047 [details] WIP: Patch to measure the overhead of DOM attributes

Kentaro Hara

Comment 19 2012-10-02 01:05:43 PDT

Created attachment 166635 [details] Patch for performance experiments. Not for review. I'll write up the performance results in days.

Kentaro Hara

Comment 20 2012-10-02 01:08:27 PDT

Created attachment 166637 [details] Benchmark tests for performance experiments I'll write up the performance results in days.

Erik Arvidsson

Comment 21 2012-10-02 07:11:38 PDT

Comment on attachment 166635 [details] Patch for performance experiments. Not for review. View in context: https://bugs.webkit.org/attachment.cgi?id=166635&action=review > Source/WebCore/bindings/v8/V8DOMConfiguration.h:85 > + (attribute.onPrototype ? prototype : instance)->SetAccessor(v8::String::NewSymbol(attribute.name), > + attribute.getter, > + attribute.setter, > + v8::External::Wrap(attribute.data), > + attribute.settings, > + attribute.attribute, > + v8::AccessorSignature::New(templ)); I'm curious why we want to do this. SetAccessor creates a magic data property. Having this on the prototype chain is a bigger semantic foul than the current incorrect bindings (it violates ES5, http://es5.github.com/#x8.12.5). Is this just to get a better understanding of the performance implications? We need to test with real accessor properties to get accurate results.

Kentaro Hara

Comment 22 2012-10-02 17:24:20 PDT

(In reply to comment #21) > I'm curious why we want to do this. SetAccessor creates a magic data property. Having this on the prototype chain is a bigger semantic foul than the current incorrect bindings (it violates ES5, http://es5.github.com/#x8.12.5). > > Is this just to get a better understanding of the performance implications? We need to test with real accessor properties to get accurate results. First of all: At present this is just for performance experiments. I can show the results in a day or so. For the future: I was planning to keep using SetAccessor. My understanding was that if SetAccessor meets the following semantics then we can use SetAccessor for correct semantics of DOM attribute getters/setters: [1] getters/setters throw a TypeError when they are called for an invalid receiver. (e.g. xhr.__proto__ = HTMLElement.prorotype; xhr.lang;) [2] JavaScript getters/setters are exposed. [1] is already implemented. We can enable the TypeError check by passing AccessorSignature to SetAccessor. I thought [2] is already implemented, but it looks like it does not work. I'll discuss it with the V8 team. I think I'm missing something. Correct me please:)

Erik Arvidsson

Comment 23 2012-10-02 20:15:34 PDT

(In reply to comment #22) > I think I'm missing something. Correct me please:) The V8 API function, SetAccessor, creates a data property (http://es5.github.com/#x8.10.2). We need a new V8 API function that creates an accessor property (http://es5.github.com/#x8.10.1). When a property is assigned to, the internal [[Put]] algorithm (http://es5.github.com/#x8.12.5) is used. If there is a data property somewhere on the prototype chain, and [[CanPut]] is true, then a new own (instance) property is defined. This means that if there is a data property, for example "id" on Element.prototype, then assigning myElement.id, will just create a new own property on myElement and the magic V8 setter should not be invoked. This is how things should work. Engines are allowed to break (almost) any spec behavior for exotic (host) objects but since we are so close it seems like a bad idea to not do this right.

Kentaro Hara

Comment 24 2012-10-02 23:59:39 PDT

(In reply to comment #23) > (In reply to comment #22) > > I think I'm missing something. Correct me please:) > > The V8 API function, SetAccessor, creates a data property (http://es5.github.com/#x8.10.2). We need a new V8 API function that creates an accessor property (http://es5.github.com/#x8.10.1). In my understanding, SetAccessor() creates a data property by default, but creates an accessor property if we pass AccessorSignature() to SetAccessor. > When a property is assigned to, the internal [[Put]] algorithm (http://es5.github.com/#x8.12.5) is used. If there is a data property somewhere on the prototype chain, and [[CanPut]] is true, then a new own (instance) property is defined. This means that if there is a data property, for example "id" on Element.prototype, then assigning myElement.id, will just create a new own property on myElement and the magic V8 setter should not be invoked. For example, the following code works fine with my patch: div.testAttrElementOnProto = "foo"; div.hasOwnProperty("testAttrElementOnProto"); // false div.__proto__.__proto__.__proto__.hasOwnProperty("testAttrElementOnProto"); // true

Kentaro Hara

Comment 25 2012-10-03 05:10:23 PDT

Here are performance results: https://docs.google.com/a/chromium.org/document/d/1ko2uA1MurSO_SFkvyefYWDcGRO7EmOmVDofdEQNbwB0/edit Summary: Half a year ago, I evaluated the performance impacts of moving DOM attributes to JavaScript prototype chains and observed ~120% performance regression. Now the V8 team has fixed all performance bugs about it, I re-evaluated the performance impacts. The good news is that now there remains only the pure overhead of prototype chain lookup. On the other hand, the bad news is that the overhead is not ignorable. Considering that one prototype chain lookup takes 0.69 ns and that the current div.nodeType takes 13.97 ns, if we move DOM attributes from div to Node.prototype, then it will cause (0.69 * 4) / 13.97 = 20% performance regression. Any wild idea to avoid the regression?

Erik Arvidsson

Comment 26 2012-10-03 07:09:29 PDT

(In reply to comment #24) > In my understanding, SetAccessor() creates a data property by default, but creates an accessor property if we pass AccessorSignature() to SetAccessor. There is no documentation supporting that claim and your Google Doc also points out that using a signature does not create a JS accessor property.

Anders Carlsson

Comment 27 2013-09-01 10:32:32 PDT

V8 is gone.

Note You need to log in before you can comment on or make changes to this bug.