| 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: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 49739 | ||
| Attachments: | |||
|
Description
Kentaro Hara
2011-12-28 00:54:47 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.
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.
Created attachment 120641 [details]
WIP: test cases
Created attachment 120661 [details]
WIP: test cases
(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 *** ap: On this bug, I would like to work on the V8 side fix of bug 49739. Created attachment 120952 [details]
WIP patch
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. 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. 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. 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... (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. Created attachment 121399 [details]
DOM micro benchmark
Uploaded a DOM micro benchmark. The result will be coming soon...
Created attachment 121401 [details]
DOM micro benchmark
Updated the DOM micro benchmark.
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%. Created attachment 123046 [details]
WIP: The OnProto patch
Created attachment 123047 [details]
WIP: Patch to measure the overhead of DOM attributes
Created attachment 166635 [details]
Patch for performance experiments. Not for review.
I'll write up the performance results in days.
Created attachment 166637 [details]
Benchmark tests for performance experiments
I'll write up the performance results in days.
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. (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:) (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. (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 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? (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. V8 is gone. |