Intent to Implement and Ship: Spec-compliant Document Accessor on the Global Object

109 views
Skip to first unread message

Daniel Vogelheim

unread,
Sep 20, 2016, 5:07:42 AM9/20/16
to blink-dev, Alfonso Peterssen, Jochen Eisinger
Contacts

Spec
The document accessor is documented in the HTML5 spec.
The WebIDL spec demands that attributes (as interface members) are implemented as JavaScript accessors.

Summary
Re-implement the 'document' accessor on the global object for improved spec compliance.

Motivation(s)
Presently, the'document' accessor on the global object is implemented as a data property. This violates the combined HTML5 + WebIDL (chapter 3, "In ECMAScript, the attributes on the IDL interfaces will be exposed as accessor properties [...]"). This was originally done because the 'document' attribute is accessed very frequently and the accessor mechanism noticeably slows this down.

We intend to implement a new mechanism, where the value of the document is stored on a hidden data property on the window object, and teach V8 to transparently access that data property when the accessor is called. The 'true' accessor is still available and fully functional, which allows for full spec compliance without performance cost. Additionally, it will enable us to remove the 'ForceSet' APi call from V8, which was used solely to implement the old behaviour.

Interoperability and Compatibility Risk
This change should increase spec compliance and bring us more in line with other browsers (which have implemented the spec all along).

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes.


OWP launch tracking bug

crbug.com/648531


Link to entry on the feature dashboard

n/a [This change is relatively minor.]


Jochen Eisinger

unread,
Sep 20, 2016, 5:22:14 AM9/20/16
to Daniel Vogelheim, blink-dev, Alfonso Peterssen
lgtm1

Kentaro Hara

unread,
Sep 20, 2016, 5:22:48 AM9/20/16
to Daniel Vogelheim, blink-dev, Alfonso Peterssen, Jochen Eisinger
non-owner LGTM


On Tue, Sep 20, 2016 at 6:07 PM, 'Daniel Vogelheim' via blink-dev <blin...@chromium.org> wrote:
Contacts

Spec
The document accessor is documented in the HTML5 spec.
The WebIDL spec demands that attributes (as interface members) are implemented as JavaScript accessors.

Summary
Re-implement the 'document' accessor on the global object for improved spec compliance.

Motivation(s)
Presently, the'document' accessor on the global object is implemented as a data property. This violates the combined HTML5 + WebIDL (chapter 3, "In ECMAScript, the attributes on the IDL interfaces will be exposed as accessor properties [...]"). This was originally done because the 'document' attribute is accessed very frequently and the accessor mechanism noticeably slows this down.

We intend to implement a new mechanism, where the value of the document is stored on a hidden data property on the window object, and teach V8 to transparently access that data property when the accessor is called. The 'true' accessor is still available and fully functional, which allows for full spec compliance without performance cost. Additionally, it will enable us to remove the 'ForceSet' APi call from V8, which was used solely to implement the old behaviour.

Also let me add that the new mechanism provides a way for V8 bindings to "cache" return values of DOM attribute getters (not limited to window.document) on the V8 side. This means that V8 can access DOM attribute getters without leaving JIT code, which will speed up the binding layer a lot.



Interoperability and Compatibility Risk
This change should increase spec compliance and bring us more in line with other browsers (which have implemented the spec all along).

Will this feature be supported on all six Blink platforms (Windows, Mac, Linux, Chrome OS, Android, and Android WebView)?

Yes.


OWP launch tracking bug

crbug.com/648531


Link to entry on the feature dashboard

n/a [This change is relatively minor.]





--
Kentaro Hara, Tokyo, Japan

Elliott Sprehn

unread,
Sep 20, 2016, 5:27:48 AM9/20/16
to Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen, Jochen Eisinger
On Tue, Sep 20, 2016 at 10:22 AM, Kentaro Hara <har...@chromium.org> wrote:
non-owner LGTM


On Tue, Sep 20, 2016 at 6:07 PM, 'Daniel Vogelheim' via blink-dev <blin...@chromium.org> wrote:
Contacts

Spec
The document accessor is documented in the HTML5 spec.
The WebIDL spec demands that attributes (as interface members) are implemented as JavaScript accessors.

Summary
Re-implement the 'document' accessor on the global object for improved spec compliance.

Motivation(s)
Presently, the'document' accessor on the global object is implemented as a data property. This violates the combined HTML5 + WebIDL (chapter 3, "In ECMAScript, the attributes on the IDL interfaces will be exposed as accessor properties [...]"). This was originally done because the 'document' attribute is accessed very frequently and the accessor mechanism noticeably slows this down.

We intend to implement a new mechanism, where the value of the document is stored on a hidden data property on the window object, and teach V8 to transparently access that data property when the accessor is called. The 'true' accessor is still available and fully functional, which allows for full spec compliance without performance cost. Additionally, it will enable us to remove the 'ForceSet' APi call from V8, which was used solely to implement the old behaviour.

Also let me add that the new mechanism provides a way for V8 bindings to "cache" return values of DOM attribute getters (not limited to window.document) on the V8 side. This means that V8 can access DOM attribute getters without leaving JIT code, which will speed up the binding layer a lot.

Does this require manual setup on our side, or will v8 just do this for all bindings now?

Daniel Vogelheim

unread,
Sep 20, 2016, 5:47:32 AM9/20/16
to Elliott Sprehn, Kentaro Hara, blink-dev, Alfonso Peterssen, Jochen Eisinger
On Tue, Sep 20, 2016 at 11:27 AM, Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 10:22 AM, Kentaro Hara <har...@chromium.org> wrote:

Motivation(s)
Presently, the'document' accessor on the global object is implemented as a data property. This violates the combined HTML5 + WebIDL (chapter 3, "In ECMAScript, the attributes on the IDL interfaces will be exposed as accessor properties [...]"). This was originally done because the 'document' attribute is accessed very frequently and the accessor mechanism noticeably slows this down.

We intend to implement a new mechanism, where the value of the document is stored on a hidden data property on the window object, and teach V8 to transparently access that data property when the accessor is called. The 'true' accessor is still available and fully functional, which allows for full spec compliance without performance cost. Additionally, it will enable us to remove the 'ForceSet' APi call from V8, which was used solely to implement the old behaviour.

Also let me add that the new mechanism provides a way for V8 bindings to "cache" return values of DOM attribute getters (not limited to window.document) on the V8 side. This means that V8 can access DOM attribute getters without leaving JIT code, which will speed up the binding layer a lot.

Does this require manual setup on our side, or will v8 just do this for all bindings now?

It definitely requires manual setup. This intent is for the 'documents' accessor only.

Doing it for all bindings would 
- require a general 'push' mechanism for all accessor-accessible data, and
- consume additional memory, since it would duplicate the values of all accessors on all wrappers.

This does not seem desirable, at least not without supporting measurements.

As Kentaro points out, it does give us a new tool to potentially speed up additional accessors. But we'll need some supporting logic for each one.

Philip Jägenstedt

unread,
Sep 20, 2016, 6:03:04 AM9/20/16
to Elliott Sprehn, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen, Jochen Eisinger
I spent a bit of time wondering if this had anything to do with prototype vs. instance or [Unforgeable], but think it doesn't. A test pedantic in web-platform-tests would to illustrate the problem would be nice. IIUC, the difference between a data property and an accessor property can be seen with Object.getOwnPropertyDescriptor, and https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4483 does behave differently in the other 3 engines. But testing the status attribute also behaves differently everywhere else.

Is document extra special, does it alone need 'ForceSet', and why? Is there a risk that we just can't move all properties on window to be accessor properties due to perf/memory, and then be stuck with a mix of both?

Jochen Eisinger

unread,
Sep 20, 2016, 6:47:49 AM9/20/16
to Philip Jägenstedt, Elliott Sprehn, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

Anyways, we already now have a mix: document and the rest. It's just that the new proposed mix here would be spec compliant.

Elliott Sprehn

unread,
Sep 20, 2016, 6:51:26 AM9/20/16
to Jochen Eisinger, Philip Jägenstedt, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

fwiw this is not my observation, element.style spends a fair amount of time inside .style, we can get 5-10% by doing:

var style = element.style;
style.foo = ...;
style.bar = ...;
etc.

Jochen Eisinger

unread,
Sep 20, 2016, 6:53:42 AM9/20/16
to Elliott Sprehn, Philip Jägenstedt, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 12:51 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

fwiw this is not my observation, element.style spends a fair amount of time inside .style, we can get 5-10% by doing:

var style = element.style;
style.foo = ...;
style.bar = ...;
etc.

5-10% of overall pageload time on a non-benchmark website?

Philip Jägenstedt

unread,
Sep 20, 2016, 6:56:41 AM9/20/16
to Jochen Eisinger, Elliott Sprehn, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
Do we have a mix in terms of observable behavior? I didn't test much, but document looked just like other attributes on window.

Elliott Sprehn

unread,
Sep 20, 2016, 7:07:43 AM9/20/16
to Jochen Eisinger, Philip Jägenstedt, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 11:53 AM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 12:51 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

fwiw this is not my observation, element.style spends a fair amount of time inside .style, we can get 5-10% by doing:

var style = element.style;
style.foo = ...;
style.bar = ...;
etc.

5-10% of overall pageload time on a non-benchmark website?

JS performance is about a lot more than just page load. :) ex. view creation or running animations. Style is particularly bad since it's accessed in big blocks, ex.


that kind of code is very common the web, where you assign many style properties in a row, if you do it for a large number of elements (ex. inline styles in react) the cost adds up. My numbers were from looking at Animometer in a profiler.

Using .classList, .attributes, and other stable object APIs all have high overhead (over other browsers) due to the bindings.

Fwiw I'm actually not sure why we keep a ptr to the .style, .classList or the .attributes object in the ElementRareData, couldn't we keep the ptr to it in the JS heap and not increase memory usage and get the perf improvement?

Jochen Eisinger

unread,
Sep 20, 2016, 7:09:31 AM9/20/16
to Philip Jägenstedt, Elliott Sprehn, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 12:56 PM Philip Jägenstedt <foo...@chromium.org> wrote:
Do we have a mix in terms of observable behavior? I didn't test much, but document looked just like other attributes on window.

ah, right. Yes, the idea is to eventually move them all (to match what the rest of the DOM looks like) https://bugs.chromium.org/p/chromium/issues/detail?id=475556

Jochen Eisinger

unread,
Sep 20, 2016, 7:11:40 AM9/20/16
to Elliott Sprehn, Philip Jägenstedt, Kentaro Hara, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 1:07 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:53 AM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 12:51 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

fwiw this is not my observation, element.style spends a fair amount of time inside .style, we can get 5-10% by doing:

var style = element.style;
style.foo = ...;
style.bar = ...;
etc.

5-10% of overall pageload time on a non-benchmark website?

JS performance is about a lot more than just page load. :) ex. view creation or running animations. Style is particularly bad since it's accessed in big blocks, ex.


that kind of code is very common the web, where you assign many style properties in a row, if you do it for a large number of elements (ex. inline styles in react) the cost adds up. My numbers were from looking at Animometer in a profiler.

Using .classList, .attributes, and other stable object APIs all have high overhead (over other browsers) due to the bindings.

Fwiw I'm actually not sure why we keep a ptr to the .style, .classList or the .attributes object in the ElementRareData, couldn't we keep the ptr to it in the JS heap and not increase memory usage and get the perf improvement?

Yeah, I mean, I'm not saying that this isn't possible. I just don't think it makes sense to apply this pattern across the board. With this specific change for document, the value needs to be pushed to v8 every time it changes. For something that changes frequently, that would be prohibitively expensive.

For getters that really always return the same value, we could just cache the result in v8 after calling once (as we already do for data properties)

Kentaro Hara

unread,
Sep 20, 2016, 7:42:45 AM9/20/16
to Jochen Eisinger, Yuki Shiino, Philip Jägenstedt, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 8:09 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 12:56 PM Philip Jägenstedt <foo...@chromium.org> wrote:
Do we have a mix in terms of observable behavior? I didn't test much, but document looked just like other attributes on window.

ah, right. Yes, the idea is to eventually move them all (to match what the rest of the DOM looks like) https://bugs.chromium.org/p/chromium/issues/detail?id=475556 

Right. yukishiino@ tried to move almost all Window's attributes to the prototype chain, but the CL was reverted due to an unexpected performance regression. (Maybe the trick we're discussing here is helpful to mitigate the performance regression.)

Kentaro Hara

unread,
Sep 20, 2016, 7:51:30 AM9/20/16
to Jochen Eisinger, Elliott Sprehn, Philip Jägenstedt, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 8:11 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 1:07 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:53 AM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 12:51 PM Elliott Sprehn <esp...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:
the thing about document what makes it special is that it's being accessed a lot. To get acceptable performance, we decided to violate the spec and make it a data property. Now a constant data property can't be changed, however, document changes its value during navigations, and so we added an API that allows for changing a value even if it's an constant.

I'm not sure whether we can move all other methods - e.g. for an accessor that returns a different value basically on every call, it wouldn't make sense. Firefox has a number of annotations in its IDL files which are used to decide which optimizations to use. Looking at Edge, there appears to be only a special case for document, which is in alignment with our measurements - for all other attributes, using the getter everytime doesn't have a measurable real-world impact.

fwiw this is not my observation, element.style spends a fair amount of time inside .style, we can get 5-10% by doing:

var style = element.style;
style.foo = ...;
style.bar = ...;
etc.

5-10% of overall pageload time on a non-benchmark website?

JS performance is about a lot more than just page load. :) ex. view creation or running animations. Style is particularly bad since it's accessed in big blocks, ex.


that kind of code is very common the web, where you assign many style properties in a row, if you do it for a large number of elements (ex. inline styles in react) the cost adds up. My numbers were from looking at Animometer in a profiler.

Using .classList, .attributes, and other stable object APIs all have high overhead (over other browsers) due to the bindings.

Fwiw I'm actually not sure why we keep a ptr to the .style, .classList or the .attributes object in the ElementRareData, couldn't we keep the ptr to it in the JS heap and not increase memory usage and get the perf improvement?

Yeah, I mean, I'm not saying that this isn't possible. I just don't think it makes sense to apply this pattern across the board. With this specific change for document, the value needs to be pushed to v8 every time it changes. For something that changes frequently, that would be prohibitively expensive.

For getters that really always return the same value, we could just cache the result in v8 after calling once (as we already do for data properties) 

There are a lot of read-only DOM attributes in the world, and the attribute caching will be very useful for speeding up those read-only attributes at least.

(Also I guess there would be few DOM attributes that change frequently -- so I guess the attribute caching will be a win for most DOM attributes.)

In any case, I believe that the attribute caching will be a strong optimization technique for v8 bindings :)

Philip Jägenstedt

unread,
Sep 20, 2016, 7:52:38 AM9/20/16
to Kentaro Hara, Jochen Eisinger, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Tue, Sep 20, 2016 at 1:42 PM Kentaro Hara <har...@chromium.org> wrote:
On Tue, Sep 20, 2016 at 8:09 PM, Jochen Eisinger <joc...@chromium.org> wrote:


On Tue, Sep 20, 2016 at 12:56 PM Philip Jägenstedt <foo...@chromium.org> wrote:
Do we have a mix in terms of observable behavior? I didn't test much, but document looked just like other attributes on window.

ah, right. Yes, the idea is to eventually move them all (to match what the rest of the DOM looks like) https://bugs.chromium.org/p/chromium/issues/detail?id=475556 

Right. yukishiino@ tried to move almost all Window's attributes to the prototype chain, but the CL was reverted due to an unexpected performance regression. (Maybe the trick we're discussing here is helpful to mitigate the performance regression.)

So, to confirm, we don't currently have a mix in observable behavior, it's just that document has an optimization that nothing else has?

I thought that the question of prototype vs. instance was separate from data property vs. accessor property, but that CLs seems to do both changes, is that right? It probably make sense to reach the final state in a single step, though.

document is on the instance because of [Unforgeable]. Still trying to understand why it should be changed to an accessor property before everything else, if it's to remove technical debt, to learn something about the compat/perf/memory risks, or something else?

Alfonso Peterssen

unread,
Sep 20, 2016, 7:58:19 AM9/20/16
to Philip Jägenstedt, Kentaro Hara, Jochen Eisinger, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev
That performance gain comes at the cost of violating the spec.
The following code works at least on Firefox and possibly other browsers (Jochen?):

val f = Object.getOwnPropertyDescriptor(window, "document");
f.get.call(window);

It doesn't on Chrome.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

Jochen Eisinger

unread,
Sep 20, 2016, 7:59:11 AM9/20/16
to Philip Jägenstedt, Kentaro Hara, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
It's just not possible to correctly implement document as a data value, as (in contrast to the other attributes) it changes over time. ForceSet on the other hand breaks the object model, which might result in optimized code not seeing the update. In that case, you'd get access to a cross-origin document which is bad.

Kentaro Hara

unread,
Sep 20, 2016, 8:04:04 AM9/20/16
to Philip Jägenstedt, Jochen Eisinger, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
(Sorry, I said a wrong thing in the previous email.)

Per the spec, Window's attributes should exist on the *instance* and be *accessor property*. Currently, Window's attributes exist on the *instance* and are *data property*. yukishiino@'s CL tried to make (almost all of) them "accessor property", keeping them on the *instance*.

In this proposal, we're going to make that change only to window.document before everything else (which should be done by yukishiino@'s CL).

document is on the instance because of [Unforgeable].

Per the spec, attributes on a [Global] object should exist on the instance regardless of [Unforgeable].

Philip Jägenstedt

unread,
Sep 20, 2016, 8:35:47 AM9/20/16
to Kentaro Hara, Jochen Eisinger, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
Oh, thanks, looks like I'm not up to date with spec land here.

Jochen, innerWidth, scrollX, screenLeft, orientation, etc. can also change are those different because they're just long/double?

Per https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4487 it looks like location also changes on navigation, but presumably it's different from document?

Jochen Eisinger

unread,
Sep 20, 2016, 8:48:47 AM9/20/16
to Philip Jägenstedt, Kentaro Hara, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
No, those are marked as [Replacable] which makes them replacable 

Per https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4487 it looks like location also changes on navigation, but presumably it's different from document?

The PutsForward attribute makes the location writable (which doesn't really make sense for a data value...)
 

Yuki Shiino

unread,
Sep 21, 2016, 2:55:19 AM9/21/16
to Jochen Eisinger, Philip Jägenstedt, Kentaro Hara, Yuki Shiino, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
Let me try to explain some background, etc.  HTH.


The current implementation is like below.

window.document attribute
  data property + no C++ callback
  (This is what ForceSet is doing.)

other Window attributes
  data property + C++ callback
  (slower than pure data property because we need to call a C++ callback.)

other attributes of non-Window interfaces
  accessor property + C++ callback
  (was slower than data property, but LAP (=Lazy Accessor Pair) improved the performance.  Maybe as fast as data property?)


This proposal is making window.document be:
  accessor property + no C++ callback
so, it conforms to the spec and it's fast because we don't need to call a C++ callback.

What my CL did was to make Window's attributes be
  accessor property + C++ callback
  (slow, especially window.document became relatively very slow.)


Correct me if I'm wrong.  My understanding is
a) LAP made accessor properties fast.  It's now comparable to data properties.
b) window.document has a special hack (ForceSet), so LAP is not enough because LAP still uses C++ callbacks.
c) This proposal makes window.document be an accessor property without C++ callback, so it will be as fast as the current special hack without violating the spec.
d) After that, we'll be able to make other Window's attributes accessor properties.  If there is a performance regression, we could use the same technique as c) for other performance-sensitive attributes, too.

Cheers,
Yuki Shiino

Kentaro Hara

unread,
Sep 21, 2016, 3:36:13 AM9/21/16
to Yuki Shiino, Jochen Eisinger, Philip Jägenstedt, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
There're a lot of discussions going on this thread :) Let me try to sum up.

1) Currently Window's attributes are implemented as data properties. We should move them to accessor properties per the spec.

2) yukishiino@'s CL tried to move them to accessor properties but was reverted due to performance regressions.

3) This Intent-to-ship is proposing to move only window.document to an accessor property in a way that doesn't regress performance (=attribute caching).

3) is clearly moving things forward, so this Intent-to-ship LGTM. Once it lands, we should try to move all the remaining Window's attributes to accessor properties (probably by relanding yukishiino@'s CL using the attribute caching).

 Does it make sense?



Jochen Eisinger

unread,
Sep 21, 2016, 6:43:02 AM9/21/16
to Kentaro Hara, Yuki Shiino, Philip Jägenstedt, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
On Wed, Sep 21, 2016 at 9:36 AM Kentaro Hara <har...@chromium.org> wrote:
There're a lot of discussions going on this thread :) Let me try to sum up.

1) Currently Window's attributes are implemented as data properties. We should move them to accessor properties per the spec.

2) yukishiino@'s CL tried to move them to accessor properties but was reverted due to performance regressions.

I wonder whether changing document as intended here is enough to avoid the regressions?

Yuki Shiino

unread,
Sep 21, 2016, 7:59:47 AM9/21/16
to Jochen Eisinger, Kentaro Hara, Yuki Shiino, Philip Jägenstedt, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
I'm sorry that I've focused on other things and didn't have time to investigate the regressions in details.  However, even if it's not enough, window.document should be the most impactful cause of the regressions, I believe.  Plus, I hope we can use the caching accessors for other performance-sensitive attributes, too.

Chris Harrelson

unread,
Sep 21, 2016, 12:34:51 PM9/21/16
to Yuki Shiino, Jochen Eisinger, Kentaro Hara, Philip Jägenstedt, Elliott Sprehn, Daniel Vogelheim, blink-dev, Alfonso Peterssen
LGTM2

Daniel Vogelheim

unread,
Sep 21, 2016, 1:19:26 PM9/21/16
to Kentaro Hara, Yuki Shiino, Jochen Eisinger, Philip Jägenstedt, Elliott Sprehn, blink-dev, Alfonso Peterssen
On Wed, Sep 21, 2016 at 9:35 AM, Kentaro Hara <har...@chromium.org> wrote:
There're a lot of discussions going on this thread :) Let me try to sum up.

1) Currently Window's attributes are implemented as data properties. We should move them to accessor properties per the spec.

2) yukishiino@'s CL tried to move them to accessor properties but was reverted due to performance regressions.

3) This Intent-to-ship is proposing to move only window.document to an accessor property in a way that doesn't regress performance (=attribute caching).

3) is clearly moving things forward, so this Intent-to-ship LGTM. Once it lands, we should try to move all the remaining Window's attributes to accessor properties (probably by relanding yukishiino@'s CL using the attribute caching).

 Does it make sense?

Thanks. Yes, I think this captures the problem(s) + proposals well!


Philip Jägenstedt

unread,
Sep 21, 2016, 3:43:16 PM9/21/16
to Daniel Vogelheim, Kentaro Hara, Yuki Shiino, Jochen Eisinger, Elliott Sprehn, blink-dev, Alfonso Peterssen
LGTM3

Thanks yukishiino@ and haraken@ for the detailed explanation. It looks like document is indeed unique here, so that making document an accessor property is a different kind of change compared to the other attributes on the Window interface. I hope that the rest can follow along soon, and it sounds like there isn't any particular reason to think that doing it in two steps is riskier than doing it all at once.
Reply all
Reply to author
Forward
0 new messages