Bug 49739

Summary: WebIDL attributes should be implemented as getters and setters on the prototype object.
Product: WebKit Reporter: Erik Arvidsson <arv@chromium.org>
Component: HTML DOMAssignee: Zan Dobersek <zandobersek@gmail.com>
Status: ASSIGNED    
Severity: Normal CC: aestes@apple.com, ap@webkit.org, cam@mcc.id.au, dglazkov@chromium.org, dominicc@chromium.org, erights@gmail.com, haraken@chromium.org, ian.eng.webkit@gmail.com, mounir@lamouri.fr, oliver@apple.com, sam@webkit.org, shadow2531@gmail.com, silverma@adobe.com, slightlyoff@chromium.org, t.brain@gmail.com, webkit@martintribe.org, zcorpan@gmail.com
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66449, 75297    
Bug Blocks: 43224, 75575, 91148, 111476    

Description From 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 #1 From 2010-11-18 10:35:47 PST -------
Also see https://bugzilla.mozilla.org/show_bug.cgi?id=608651 and http://code.google.com/p/chromium/issues/detail?id=43394
------- Comment #2 From 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 From 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 From 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 From 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 From 2011-06-19 03:27:15 PST -------
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 From 2011-06-19 13:21:03 PST -------
(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 From 2011-06-19 17:08:39 PST -------
(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 From 2011-06-19 17:15:56 PST -------
(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 From 2011-06-19 17:16:58 PST -------
(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 From 2011-06-19 18:00:14 PST -------
(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 From 2011-06-19 18:22:27 PST -------
(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 From 2011-12-29 06:44:02 PST -------
*** Bug 75297 has been marked as a duplicate of this bug. ***
------- Comment #14 From 2011-12-29 06:44:59 PST -------
*** Bug 68002 has been marked as a duplicate of this bug. ***
------- Comment #15 From 2012-03-03 14:38:09 PST -------
*** Bug 80060 has been marked as a duplicate of this bug. ***
------- Comment #16 From 2012-03-07 00:10:12 PST -------
*** Bug 18737 has been marked as a duplicate of this bug. ***
------- Comment #17 From 2012-03-07 18:21:24 PST -------
*** Bug 12721 has been marked as a duplicate of this bug. ***
------- Comment #18 From 2012-03-24 00:19:19 PST -------
*** Bug 82031 has been marked as a duplicate of this bug. ***
------- Comment #19 From 2014-04-11 07:35:47 PST -------
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.