Install nodeType attribute as constant in function tempalte

Install nodeType attribute as constant in function tempalte

BUG=None

[Pan, 2014-09-09 08:36:58]

pan.deng@intel.com changed reviewers:
+ haraken@chromium.org, tonyg@chromium.org

[Pan, 2014-09-09 08:36:58]

@haraken,

This CL describes the idea to improve DOM API node.nodeType access performance. 
nodeType for a node category is defined in W3C DOM spec, for a node instance,
nodeType is readonly which means it won't be changed during its lifetime.
According to above, we have a chance to install nodeType attribute as constant
rather than a function property in function tmplate, then, during the
jsobj.nodeType invocation, C++ callback functions in V8Node, and the virtual
function nodeType() in blink will be saved.
The perf gain is measured on x86/ubuntu 12.04-64 Chrome, ~5% improvement is
observed on Dromaeo core dom query, ~5%
improvement is observed on jslib DOM Traversal (jQuery and prototype), and ~3%
gains observed on BrowserMark2.0 DOM advanced_search. The first case directly
invokes nodeType API in its test, while the later two doesn't, they were
improved because nodeType is frequently checked in JQuery library, it is the
reason I think it is worthy to optimize it although its own path length is not
that high.
The downside of this CL is the "hasOwnProperty('nodeType')" behavior, with this
CL, nodeType belongs to concrete node types(Document, Element,etc), current
status in Chromium is it belongs to "instance", actually, neither of them is
correct, it should belong to Node. It will be great if you provide some idea to
solve this problem, anyway, I think this CL is not a backward step. :)

thanks a lot for your help.
Pan

[haraken, 2014-09-09 09:13:26]

haraken@chromium.org changed reviewers:
+ dcarney@chromium.org, jochen@chromium.org

[haraken, 2014-09-09 09:13:26]

Thanks for looking into the performance of DOM bindings.

That being said, this approach does not LGTM.

nodeType is defined as a readonly DOM attribute in the spec. Then we have to
define it as a readonly DOM attribute in our IDL file according to the spec.
Spec conformance is important.

> The downside of this CL is the "hasOwnProperty('nodeType')" behavior, with
this
> CL, nodeType belongs to concrete node types(Document, Element,etc), current
> status in Chromium is it belongs to "instance", actually, neither of them is
> correct, it should belong to Node. It will be great if you provide some idea
to
> solve this problem, anyway, I think this CL is not a backward step. :)

I agree that the result of hasOwnProperty is already broken in Chrome, but the
hasOwnProperty bug is going to be fixed soon
(https://codereview.chromium.org/447523003/). Even if we land this CL now, the
performance improvement will be lost when we fix the hasOwnProperty bug in the
future.

[blink-reviews, 2014-09-09 14:03:39]

Brainstorming...

In the future we could add a V8 private symbol on the different prototypes
and then have a simple pure js getter for Node.prototype.nodeType that just
returns that.
On Sep 9, 2014 5:13 AM, <haraken@chromium.org> wrote:

> Thanks for looking into the performance of DOM bindings.
>
> That being said, this approach does not LGTM.
>
> nodeType is defined as a readonly DOM attribute in the spec. Then we have
> to
> define it as a readonly DOM attribute in our IDL file according to the
> spec.
> Spec conformance is important.
>
>  The downside of this CL is the "hasOwnProperty('nodeType')" behavior, with
>>
> this
>
>> CL, nodeType belongs to concrete node types(Document, Element,etc),
>> current
>> status in Chromium is it belongs to "instance", actually, neither of them
>> is
>> correct, it should belong to Node. It will be great if you provide some
>> idea
>>
> to
>
>> solve this problem, anyway, I think this CL is not a backward step. :)
>>
>
> I agree that the result of hasOwnProperty is already broken in Chrome, but
> the
> hasOwnProperty bug is going to be fixed soon
> (https://codereview.chromium.org/447523003/). Even if we land this CL
> now, the
> performance improvement will be lost when we fix the hasOwnProperty bug in
> the
> future.
>
>
> https://codereview.chromium.org/555723002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tonyg, 2014-09-09 14:35:00]

Just out of curiosity, do we know if nodeType is relevant on the speedometer
benchmark?

[Pan, 2014-09-12 01:09:39]

On 2014/09/09 14:35:00, tonyg wrote:
> Just out of curiosity, do we know if nodeType is relevant on the speedometer
> benchmark?

nodeType is not that hot in speedometer.

thanks
Pan

[haraken, 2014-09-12 01:22:43]

On 2014/09/12 01:09:39, Pan wrote:
> On 2014/09/09 14:35:00, tonyg wrote:
> > Just out of curiosity, do we know if nodeType is relevant on the speedometer
> > benchmark?
> 
> nodeType is not that hot in speedometer.

Just FYI, nodeType was the top-most DOM attribute used in top 1K websites when
we investigated two years ago. nodeType explained 32% of all the DOM
attribute/method usage. (This doesn't mean that nodeType explained 32% of all
the *execution time* consumed in DOM attributes/methods.)

[Pan, 2014-09-12 09:08:58]

pan.deng@intel.com changed reviewers:
+ arv@chromium.org

[Pan, 2014-09-12 09:08:59]

I didn't try it in various websites, but realized it is hot in several
benchmarks. 
@arv, could you please provide more information of your idea? 

thanks
Pan

[arv (Not doing code reviews), 2014-09-12 15:44:04]

On 2014/09/12 09:08:59, Pan wrote:
> @arv, could you please provide more information of your idea? 

V8 has something called private symbols (these are like ES6 symbols but never
exposed to any user script in any way). We could define a private symbol for the
nodeType and then use that in the getter. The following code should work
today[*] if we remove the instance nodeType property from the bindings.

var nodeTypeSymbol = NEW_PRIVATE('nodeTypeSymbol');
Object.defineProperty(Node.prototype, 'nodeType', {
  get: function() {
    // add instance type check?
    return this[nodeTypeSymbol];
  },
  enumerable: true,
  configurable: true
});
Element.prototype[nodeTypeSymbol] = Node.ELEMENT_NODE;
Attr.prototype[nodeTypeSymbol] = Node.ATTRIBUTE_NODE;
Text.prototype[nodeTypeSymbol] = Node.TEXT_NODE;
...

Now V8 does not have to invoke any C++ callbacks and the whole code can be
jitted.

[*] We need a way to expose v8 natives to Blink.

We should also have a way to do true Blink in JS =P

To test this you could use a public symbol. It should be good enough for doing
benchmarks.

[Pan, 2014-09-24 00:19:12]

On 2014/09/12 15:44:04, arv wrote:
> On 2014/09/12 09:08:59, Pan wrote:
> > @arv, could you please provide more information of your idea? 
> 
> V8 has something called private symbols (these are like ES6 symbols but never
> exposed to any user script in any way). We could define a private symbol for
the
> nodeType and then use that in the getter. The following code should work
> today[*] if we remove the instance nodeType property from the bindings.
/////////////////////////////////////////////////////////////////////// 1st
section start
> var nodeTypeSymbol = NEW_PRIVATE('nodeTypeSymbol');
> Object.defineProperty(Node.prototype, 'nodeType', {
>   get: function() {
>     // add instance type check?
>     return this[nodeTypeSymbol];
>   },
>   enumerable: true,
>   configurable: true
> });
> Element.prototype[nodeTypeSymbol] = Node.ELEMENT_NODE;
> Attr.prototype[nodeTypeSymbol] = Node.ATTRIBUTE_NODE;
> Text.prototype[nodeTypeSymbol] = Node.TEXT_NODE;
> ...
> 
///////////////////////////////////////////////////////////////////////////////////
1st section end
> Now V8 does not have to invoke any C++ callbacks and the whole code can be
> jitted.

> [*] We need a way to expose v8 natives to Blink.
> 
> We should also have a way to do true Blink in JS =P
> 
> To test this you could use a public symbol. It should be good enough for doing
> benchmarks.

@arv, great thanks for your reply, I'm not familiar with V8 :(, so more
questions here.
1, By "grep" the v8 code base, I found few "NEW_PRIVATE" keywords, as my
understanding, the place that V8 interact with node should be in binding, then
where the 1st section code should be placed?

2,
> [*] We need a way to expose v8 natives to Blink. 
  expose what kind of functionality to blink? through v8 API?

3, is there similar mechanism in Blink in JS project? if true, good for me to
learn from it.


thanks
Pan

[arv (Not doing code reviews), 2014-09-29 15:38:26]

On 2014/09/24 00:19:12, Pan wrote:
> On 2014/09/12 15:44:04, arv wrote:
> > On 2014/09/12 09:08:59, Pan wrote:
> > > @arv, could you please provide more information of your idea? 
> > 
> > V8 has something called private symbols (these are like ES6 symbols but
never
> > exposed to any user script in any way). We could define a private symbol for
> the
> > nodeType and then use that in the getter. The following code should work
> > today[*] if we remove the instance nodeType property from the bindings.
> /////////////////////////////////////////////////////////////////////// 1st
> section start
> > var nodeTypeSymbol = NEW_PRIVATE('nodeTypeSymbol');
> > Object.defineProperty(Node.prototype, 'nodeType', {
> >   get: function() {
> >     // add instance type check?
> >     return this[nodeTypeSymbol];
> >   },
> >   enumerable: true,
> >   configurable: true
> > });
> > Element.prototype[nodeTypeSymbol] = Node.ELEMENT_NODE;
> > Attr.prototype[nodeTypeSymbol] = Node.ATTRIBUTE_NODE;
> > Text.prototype[nodeTypeSymbol] = Node.TEXT_NODE;
> > ...
> > 
>
///////////////////////////////////////////////////////////////////////////////////
> 1st section end
> > Now V8 does not have to invoke any C++ callbacks and the whole code can be
> > jitted.
> 
> > [*] We need a way to expose v8 natives to Blink.
> > 
> > We should also have a way to do true Blink in JS =P
> > 
> > To test this you could use a public symbol. It should be good enough for
doing
> > benchmarks.
> 
> @arv, great thanks for your reply, I'm not familiar with V8 :(, so more
> questions here.
> 1, By "grep" the v8 code base, I found few "NEW_PRIVATE" keywords, as my
> understanding, the place that V8 interact with node should be in binding, then
> where the 1st section code should be placed?

Our binding code does not have a place where we could run js code to set up the
environment.

> 2,
> > [*] We need a way to expose v8 natives to Blink. 
>   expose what kind of functionality to blink? through v8 API?

Same as 1. We have no way to run js code when a context is created.
 
> 3, is there similar mechanism in Blink in JS project? if true, good for me to
> learn from it.

Blink in JS is only about implementation and all the calls always goes through
the C++ bindings. It is not going to help here.

> 
> 
> thanks
> Pan

[tonyg, 2015-01-14 18:19:04]

tonyg@chromium.org changed reviewers:
- tonyg@chromium.org