Bug 49739

Summary: WebIDL attributes should be implemented as getters and setters on the prototype object.
Product: WebKit Reporter: Erik Arvidsson <arv>
Component: HTML DOMAssignee: Zan Dobersek <zan>
Status: ASSIGNED ---    
Severity: Major CC: aestes, ap, cam, dglazkov, dominicc, erights, haraken, ian.eng.webkit, joepeck, marfalkov, mike, mounir, oliver, p.jacquemart, sam, shadow2531, silverma, slightlyoff, t.brain, webkit, zcorpan, zimbabao
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66449, 75297    
Bug Blocks: 75575, 111476, 43224, 91148    

Description Erik Arvidsson 2010-11-18 10:12:00 PST
This was discussed and agreed upon at the joint WebIDL ECMA TC39 meeting.

Today, we store methods on the prototype but we always store the attributes on the instance.

Current behavior:

Object.getOwnPropertyDescriptor(document.body, 'nodeType') => {"value":1,"writable":true,"enumerable":true,"configurable":true}
Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType') => undefined

Expected behavior:

Object.getOwnPropertyDescriptor(document.body, 'nodeType') => undefined
Object.getOwnPropertyDescriptor(Node.prototype, 'nodeType') => {
  get: function () { [native code] },
  enumerable: true,
  configurable: true
}
Comment 2 Sam Weinig 2010-11-21 18:39:56 PST
Do we have an idea about how to do this without slowing down DOM property access in the common case?
Comment 3 Erik Arvidsson 2010-11-22 13:27:10 PST
(In reply to comment #2)
> Do we have an idea about how to do this without slowing down DOM property access in the common case?

This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]?

How do you optimize getters/setters today?

How do you optimize [[Get]] when present on a [[Prototype]]?
Comment 4 Sam Weinig 2010-11-22 14:00:09 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Do we have an idea about how to do this without slowing down DOM property access in the common case?
> 
> This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]?
> 
> How do you optimize getters/setters today?
> 
> How do you optimize [[Get]] when present on a [[Prototype]]?

My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft).  The answer is probably no.
Comment 5 Oliver Hunt 2010-11-22 14:22:09 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Do we have an idea about how to do this without slowing down DOM property access in the common case?
> > 
> > This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]?
> > 
> > How do you optimize getters/setters today?
> > 
> > How do you optimize [[Get]] when present on a [[Prototype]]?
> 
> My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft).  The answer is probably no.

I'm unsure how much impact this will have on setters, as i'm 90% sure that setters are completely uncached currently.

Getters will be hit by additional branches to validate the proto chain which is unfortunate.

The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call.  Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/

Additionally JSC API probably won't be able to do this for custom getters, etc

I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet.
Comment 6 Dominic Cooney 2011-06-19 03:27:15 PDT
I intend to investigate the actual performance impact of this change in JSC.

I think that there is a limitation in the V8 API to even be able to specify properties with getters and setters, so I have filed <http://code.google.com/p/v8/issues/detail?id=1482> for that.
Comment 7 Oliver Hunt 2011-06-19 13:21:03 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Do we have an idea about how to do this without slowing down DOM property access in the common case?
> > > 
> > > This is probably better answered by someone who knows JSC better but isn't this just the same as accessing normal JavaScript objects that have getters on their [[Prototype]]?
> > > 
> > > How do you optimize getters/setters today?
> > > 
> > > How do you optimize [[Get]] when present on a [[Prototype]]?
> > 
> > My question is not if/how to optimize getters on a prototype, but rather, if we can make this change without a regression in performance (through some witchcraft).  The answer is probably no.
> 
> I'm unsure how much impact this will have on setters, as i'm 90% sure that setters are completely uncached currently.

Correction: I know that setters aren't cached.  Assigning to dom properties would be hammered, but they are theoretically already slow.  If we added caching to setters we would probably end up with a net win.

> 
> Getters will be hit by additional branches to validate the proto chain which is unfortunate.

This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path.  We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object)

> 
> The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call.  Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/
>

A virtual call is no longer necessary but there's still going to be an additional (unavoidable) cost here, but also we'll nee to go through _every_ custom getter or setter and ensure correct behaviour.  
 
> Additionally JSC API probably won't be able to do this for custom getters, etc
This has been dealt with already by geoff -- API objects kill optimisation :(

> 
> I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet.

This is really an issue that needs to be resolves -- do index properties move to the prototype or do they stay own properties?
Comment 8 Dominic Cooney 2011-06-19 17:08:39 PDT
(In reply to comment #7)

Thanks for the insight.

> Correction: I know that setters aren't cached.  Assigning to dom properties would be hammered, but they are theoretically already slow.  If we added caching to setters we would probably end up with a net win.
> 
> > Getters will be hit by additional branches to validate the proto chain which is unfortunate.
> 
> This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path.  We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object)

Could you point me to where this cache is in JSC?
 
> > The big change will be the property accessors in the dom will need to do type checks and that's an additional virtual call.  Could be fixed if class info was hung off of structure but then Structure becomes much larger. :-/
> 
> A virtual call is no longer necessary but there's still going to be an additional (unavoidable) cost here, but also we'll nee to go through _every_ custom getter or setter and ensure correct behaviour.  
> 
> > Additionally JSC API probably won't be able to do this for custom getters, etc
> This has been dealt with already by geoff -- API objects kill optimisation :(
> 
> > I believe classes with index property accessors are going to magically become proxies, but i don't believe the semantics for that is sufficiently described yet.
> 
> This is really an issue that needs to be resolves -- do index properties move to the prototype or do they stay own properties?

According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties>
Comment 9 Cameron McCormack 2011-06-19 17:15:56 PDT
(In reply to comment #8)
> According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties>

Please look at the copy in dev.w3.org, which has changed somewhat since the last TR publication:

http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties

This section did change just recently, from being a description of when properties get set and removed from the object, to being a description of [[GetOwnProperty]]/[[DefineOwnProperty]] behaviour to expose indexed and named properties.

There was a thread on public-script-coord around this recently:

http://www.w3.org/mid/20110503052431.GN2576@wok.mcc.id.au
Comment 10 Oliver Hunt 2011-06-19 17:16:58 PDT
(In reply to comment #8)
> (In reply to comment #7)
> 
> Thanks for the insight.
> 
> > Correction: I know that setters aren't cached.  Assigning to dom properties would be hammered, but they are theoretically already slow.  If we added caching to setters we would probably end up with a net win.
> > 
> > > Getters will be hit by additional branches to validate the proto chain which is unfortunate.
> > 
> > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path.  We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object)
> 
> Could you point me to where this cache is in JSC?

What cache?

> > This is really an issue that needs to be resolves -- do index properties move to the prototype or do 
they stay own properties?
> 
> According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties>

Do they end up as getters or values?
Comment 11 Dominic Cooney 2011-06-19 18:00:14 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > 
> > Thanks for the insight.
> > 
> > > Correction: I know that setters aren't cached.  Assigning to dom properties would be hammered, but they are theoretically already slow.  If we added caching to setters we would probably end up with a net win.
> > > 
> > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate.
> > > 
> > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path.  We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object)
> > 
> > Could you point me to where this cache is in JSC?
> 
> What cache?

When you say setters aren’t cached, but because getters/setters come in pairs it could be possible to remove a branch from the "current cached path", what specifically are you referring too? I am still getting up to speed on JSC…

> > > This is really an issue that needs to be resolves -- do index properties move to the prototype or do 
> they stay own properties?
> > 
> > According to the Web IDL spec, indexed properties are on the host object, not the prototype <http://www.w3.org/TR/WebIDL/#indexed-properties>
> 
> Do they end up as getters or values?

From the link Cam sent, it looks like they appear to JS to be values in the current editor’s draft: <http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties>

If a host object implements an interface that supports indexed or named properties, the object will appear to have additional properties that correspond to the object’s indexed and named properties. These properties are not “real” own properties on the object, but are made to look like they are by being exposed by the [[GetOwnProperty]] internal method.

…

The internal [[GetOwnProperty]] method of every host object O that implements an interface that supports indexed or named properties must behave as follows … Let desc be a newly created property descriptor ([ECMA-262], section 8.10) with no fields. … Set desc.[[Value]] to the result of converting value to an ECMAScript value.
Comment 12 Oliver Hunt 2011-06-19 18:22:27 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > 
> > > Thanks for the insight.
> > > 
> > > > Correction: I know that setters aren't cached.  Assigning to dom properties would be hammered, but they are theoretically already slow.  If we added caching to setters we would probably end up with a net win.
> > > > 
> > > > > Getters will be hit by additional branches to validate the proto chain which is unfortunate.
> > > > 
> > > > This will suck, but given that they're getter/setter pairs that we provide we might be able to remove at least _a_ branch from the current cached path.  We should also be able to avoid constructing an actual function wrapper until we absolutely have to (getOwnPropertyDescriptor requires us to provide the function object)
> > > 
> > > Could you point me to where this cache is in JSC?
> > 
> > What cache?
> 
> When you say setters aren’t cached, but because getters/setters come in pairs it could be possible to remove a branch from the "current cached path", what specifically are you referring too? I am still getting up to speed on JSC…

JSC has an internal idiosyncrasy/bug that means an object's structure is influenced by a new property being added as a getter or setter, but subsequently changing that getter or setter does not change the structure.  This means we have to do a null check in the "cached" path.  For builtin getters/setters we know they're well behaved and should be able to drop that bit of stupid, even if we don't fix the main getter/setter stupidity
Comment 13 Kentaro Hara 2011-12-29 06:44:02 PST
*** Bug 75297 has been marked as a duplicate of this bug. ***
Comment 14 Kentaro Hara 2011-12-29 06:44:59 PST
*** Bug 68002 has been marked as a duplicate of this bug. ***
Comment 15 Marc Silverman 2012-03-03 14:38:09 PST
*** Bug 80060 has been marked as a duplicate of this bug. ***
Comment 16 Gavin Barraclough 2012-03-07 00:10:12 PST
*** Bug 18737 has been marked as a duplicate of this bug. ***
Comment 17 Gavin Barraclough 2012-03-07 18:21:24 PST
*** Bug 12721 has been marked as a duplicate of this bug. ***
Comment 18 Alexey Proskuryakov 2012-03-24 00:19:19 PDT
*** Bug 82031 has been marked as a duplicate of this bug. ***
Comment 19 Erik Arvidsson 2014-04-11 07:35:47 PDT
I just noticed that the descriptors have changed. However, both get and set are undefined.

Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title');
{
  configurable: false,
  enumerable: true,
  get: undefined,
  set: undefined
}

This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon.

P.S. The only thing correct in that descriptor is the enumerable bit.
Comment 20 Zan Dobersek 2014-05-23 10:52:57 PDT
(In reply to comment #19)
> I just noticed that the descriptors have changed. However, both get and set are undefined.
> 
> Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title');
> {
>   configurable: false,
>   enumerable: true,
>   get: undefined,
>   set: undefined
> }
> 
> This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon.

This seems to be due to this change, by Oliver:
http://trac.webkit.org/changeset/163035

> 
> P.S. The only thing correct in that descriptor is the enumerable bit.

Configurability of WebIDL attributes is covered in bug #122018.
Comment 21 Oliver Hunt 2014-05-23 11:00:39 PDT
(In reply to comment #19)
> I just noticed that the descriptors have changed. However, both get and set are undefined.
> 
> Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title');
> {
>   configurable: false,
>   enumerable: true,
>   get: undefined,
>   set: undefined
> }
> 
> This is pretty bad since it looks like you support accessors but you really do not. I hope you are not shipping anything with this any time soon.
> 
> P.S. The only thing correct in that descriptor is the enumerable bit.

It is preferable to get the property in the right place, even if the accessors are not initially accessible.  Even the simple act of moving the property has been problematic due to sites doing stupid things with them. At least by getting these onto the prototype it becomes possible to introduce your own shim object to intercept if you really want to.

As far as exposing the accessor functions we'll happily accept patches but the problem is avoiding huge amounts of code bloat
Comment 22 Erik Arvidsson 2014-07-01 13:07:30 PDT
This is now breaking real world sites because the code never assumes a falsy value for get/set

```js
if (descriptor.writable || descriptor.set) {
```

Like I've said before, the current behavior is a violation of ECMA 262

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-get-p-receiver

Olli, would you accept patches that reverts this behavior?
Comment 23 Sam Weinig 2014-07-01 13:58:51 PDT
(In reply to comment #22)
> This is now breaking real world sites because the code never assumes a falsy value for get/set
> 

Which sites?
Comment 24 Erik Arvidsson 2014-07-01 14:17:43 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > This is now breaking real world sites because the code never assumes a falsy value for get/set
> > 
> 
> Which sites?

Anything based on Polymer but most likely other sites as well.

www.chromestatus.com/features
zeitgeist-globe.appspot.com/
Comment 25 Joseph Fneisz 2014-08-08 11:02:15 PDT
http://webcomponents.org/
Comment 26 Joseph Pecoraro 2015-02-13 17:43:36 PST
(In reply to comment #21)
> (In reply to comment #19)
> > I just noticed that the descriptors have changed. However, both get and set are undefined.
> > 
> > Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'title');
> > {
> >   configurable: false,
> >   enumerable: true,
> >   get: undefined,
> >   set: undefined
> > }
> > 
>
> <snip>
>
> As far as exposing the accessor functions we'll happily accept patches but
> the problem is avoiding huge amounts of code bloat

I believe I am addressing this in:
<https://webkit.org/b/140575> Native Bindings Descriptors are Incomplete

That said, there are still properties that haven't moved to the prototype chain. I won't be addressing those, and I'll continue outputting a broken descriptor. I filed:
<https://webkit.org/b/141585> Some IDL attributes appear on the instances instead of on prototypes