Add a new api in ObjectTemplate to prohibit interceptor for V8DOM binding

Add a new api in ObjectTemplate to prohibit interceptor for v8DOM binding

JS code HTMLCollection.length jited to go to interceptor callback,
And In the native, the interceptor callback will first check whether
the property is HasRealNamedProperty(), For HTMLCollection.length,
it is true, so the interceptor callback fails and later go to the real
length-getter handler. So for very length-getter, it needs the extra
overhead of name lookup as the slow-path do.

This patch adds a new API in ObjectTemplate to prohibit the interceptor
for v8 binding of HTMLCollection, and improve chrome
http://dromaeo.com/?dom-query by 20%.

[closed since it was superseded by dcarney's appending interceptors]

BUG=v8:3464
LOG=Y

[yaojie.yan, 2014-07-25 11:18:01]

This is the v8 side implementation, the binding patch is
https://codereview.chromium.org/423473002
The issue opened in is https://code.google.com/p/v8/issues/detail?id=3464

Could you help review them, and give your comments, thanks

BRs
yaojie.yan@intel.com/yaojie.yan@gmail.com

[yaojie.yan, 2014-08-01 15:13:22]

danno@, dcarney@, would you help to review this patch?

[Toon Verwaest, 2014-08-01 15:19:53]

This patch is at least incomplete since it skips the LookupIterator entirely.

What's the rationale behind this patch? Is this an alternative to having
interceptors that first check for "real" properties?

[yaojie.yan, 2014-08-01 15:40:55]

On 2014/08/01 15:19:53, Toon Verwaest wrote:
> This patch is at least incomplete since it skips the LookupIterator entirely.
> 
> What's the rationale behind this patch? Is this an alternative to having
> interceptors that first check for "real" properties?

This patch is try to improve the HTMLCollection.length binding performance.
HTMLCollection has the NamedPropertyHandler(so we can code js like below:)

elem1 = document.forms["myForm"]; // document.forms return HTMLCollection, with
the NamedPropertyHandler callback, we can get the element by specifying an item
name
elem2 = document.forms.namedItem("myForm");

But for the real name "length", it always first go to the interceptor, in the
interceptor callback, it find the length is the real name(by call
HasRealNamedProperty(), in
out/Debug/gen/blink/bindings/core/v8/V8HTMLCollection.cpp), and it fails, later
it fallback to the real name getter. it leads to much overhead of name lookup.
So we try to add a flag to disable the interceptor for "length" name(another
patch to disable the interceptor is in blink part).

[Toon Verwaest, 2014-08-01 15:51:31]

For exactly this reason we are thinking of introducing a new kind of interceptor
that always first does a local lookup. This patch is a limited version of the
reverse, so I'd oppose to landing it.

[yaojie.yan, 2014-08-01 16:14:39]

On 2014/08/01 15:51:31, Toon Verwaest wrote:
> For exactly this reason we are thinking of introducing a new kind of
interceptor
> that always first does a local lookup. This patch is a limited version of the
> reverse, so I'd oppose to landing it.

yes, I originally chosen that method for the compatibility purpose. I also
thought of the partial interceptor, just interceptor name without real name, but
it need some change with the v8 api, for an example: add a flag in an new api 
  bool SetAccessor(Handle<String> name,
                   AccessorGetterCallback getter,
                   AccessorSetterCallback setter = 0,
                   Handle<Value> data = Handle<Value>(),
                   AccessControl settings = DEFAULT,
                   PropertyAttribute attribute = None,
bool partialInterceptor);  // Not interceptor if has realName

[Toon Verwaest, 2014-08-01 17:22:20]

No, I mean we shouldn't touch api accessors at all, but rather have specialized
interceptors that tell the runtime to first check for real named properties
before calling the interceptor. That applies to all properties, not just
collection.length.

[yaojie.yan, 2014-08-02 01:15:44]

On 2014/08/01 17:22:20, Toon Verwaest wrote:
> No, I mean we shouldn't touch api accessors at all, but rather have
specialized
> interceptors that tell the runtime to first check for real named properties
> before calling the interceptor. That applies to all properties, not just
> collection.length.

ok,thanks, I got it, I can work out it with the new interceptor.

[yaojie.yan, 2014-09-12 15:13:15]

On 2014/08/02 01:15:44, yaojie.yan wrote:
> On 2014/08/01 17:22:20, Toon Verwaest wrote:
> > No, I mean we shouldn't touch api accessors at all, but rather have
> specialized
> > interceptors that tell the runtime to first check for real named properties
> > before calling the interceptor. That applies to all properties, not just
> > collection.length.
> 
> ok,thanks, I got it, I can work out it with the new interceptor.

verwaest@, could you have a look at the new patch, which add a new api to
FunctionTemplate::UninterceptorRealName() to prohibit interceptor for real name
property. In v8 binding side (that is
out/Release/gen/blink/bindings/core/v8/V8HTMLCollection.cpp) , to call:
  functionTemplate->InstanceTemplate()->UninterceptorRealName(true)
will prohibit the interceptor.

[Toon Verwaest, 2014-09-15 11:33:22]

Seems like a step in the right direction.

Q: Do you really only want to unmask properties on the holder itself; or rather
on the entire chain? If the entire chain, how does that work with access checks?

I don't really like the API design to be honest. I leave it to dcarney@ to
decide on that part.

[yaojie.yan, 2014-09-16 08:38:49]

On 2014/09/15 11:33:22, Toon Verwaest wrote:
> Seems like a step in the right direction.
> 
> Q: Do you really only want to unmask properties on the holder itself; or
rather
> on the entire chain? If the entire chain, how does that work with access
checks?

I think we only need to unmask properties on the holder for optimizing such
case, and I don't think there is any requirement to disable the interceptor in
the entire chain, am I right?

> 
> I don't really like the API design to be honest. I leave it to dcarney@ to
> decide on that part.

In order to minimize the change in v8.h, I can only think of to unset the flag
of interceptor with some api, what’s your comments, dcarney@, 

Thanks a lot.

[dcarney, 2014-10-17 08:21:40]

> In order to minimize the change in v8.h, I can only think of to unset the flag
> of interceptor with some api, what’s your comments, dcarney@, 

we need to make a number of other changes to interceptors.  after that, this
change will become simple and won't complicate the api in any way

[yaojie.yan, 2014-10-23 01:42:26]

On 2014/10/17 08:21:40, dcarney wrote:
> > In order to minimize the change in v8.h, I can only think of to unset the
flag
> > of interceptor with some api, what’s your comments, dcarney@, 
> 
> we need to make a number of other changes to interceptors.  after that, this
> change will become simple and won't complicate the api in any way

Are these change under the progress? or if there is any work I can do to land
this optimization?, we think this patch is helpful for blink to match with
safari on Dromaeo benchmark(because Safari has no runtime overhead of lookup
name property in HTMLCollection.length case, Safari is 4x faster than chrome in
this case).

Thanks
Yaojie