binding: Reimplements V8HiddenValue as V8PrivateProperty.

binding: Reimplements V8HiddenValue as V8PrivateProperty.

V8HiddenValue is very slow.  This CL implements it again as
V8PrivateProperty so that it will be efficient.  Also makes it clear
which interface and which attribute, etc. is using which private symbol.
Note that "Class#property" is recommended as symbol names by V8.

Following CLs will replace V8HiddenValue with V8PrivateProperty gradually.

BUG=611864
Committed: https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa
Cr-Commit-Position: refs/heads/master@{#396694}

[Yuki, 2016-05-23 13:41:50]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2016-05-23 13:41:50]

Could you review this CL?

[haraken, 2016-05-23 22:03:58]

The change looks good.

Did you get some performance results with this CL?

[Yuki, 2016-05-24 08:00:07]

On 2016/05/23 22:03:58, haraken wrote:
> The change looks good.
> 
> Did you get some performance results with this CL?

I ran a benchmark with document-implementation.html, which is one of tests
regressed with [SameObject].

[SameObject] implemented with V8HiddenValue
     36 runs / sec

[SameObject] implemented with V8PrivateProperty
    134 runs / sec    // 4+ times faster!!!

Without [SameObject]
    486 runs / sec    // ah, umm...

Well, V8PrivateProperty is faster than V8HiddenValue, though it still costs.

[haraken, 2016-05-24 17:33:53]

4x speed-up is already a big win. I'm fine with landing this CL even if it
doesn't fix all of the perf issues :)

Also let's make sure that this CL addresses the perf regression of kbr's WebGL
CL. +kbr FYI.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:22: if
(!LIKELY(v8::String::NewFromOneByte(isolate, reinterpret_cast<const
uint8_t*>(symbol), v8::NewStringType::kNormal,
static_cast<int>(length)).ToLocal(&str))) {

Can we use v8CallOrCrash?

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:85:
v8::Local<v8::Value> getFromMainWorld(ScriptState*, ScriptWrappable*);

Add a comment that this method should not be used unless you understand what
you're doing. It's dangerous to return a main world's wrapper to another world.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:94: v8::Isolate*
m_isolate;

Instead of caching m_isolate, can we cache v8::Context (because Symbol should be
associated with v8::Context)? Then you don't need to call
m_isolate->GetCurrentContext() in get().

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:100: return
std::unique_ptr<V8PrivateProperty>(new V8PrivateProperty());

base::wrapUnique ?

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:107: if
(privateProp->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName).isEmpty())
{ \

Add UNLIKELY?

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/MessageEvent.cpp:228:
V8PrivateProperty::getMessageEventCachedData(isolate).set(wrapper,
v8String(isolate, dataAsString()));

Another possible API would be:

  V8PrivateProperty::get(context, V8PrivateProperty::messageEventCacheData());
  V8PrivateProperty::set(context, V8PrivateProperty::messageEventCacheData());

V8PrivateProperty::messageEventCacheData() returns v8::Local<v8::Private>. With
this API, you don't need to create a Symbol object, which might decrease the
overhead of accessing private properties (I'm not sure).

[Ken Russell (switch to Gerrit), 2016-05-24 21:49:18]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-05-24 21:49:19]

Thanks for the CC:.

The overall direction seems fine. v8::Private is marked an experimental API.
Should it be taken out of experimental mode?

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/MessageEvent.cpp:228:
V8PrivateProperty::getMessageEventCachedData(isolate).set(wrapper,
v8String(isolate, dataAsString()));
On 2016/05/24 17:33:53, haraken wrote:
> 
> Another possible API would be:
> 
>   V8PrivateProperty::get(context, V8PrivateProperty::messageEventCacheData());
>   V8PrivateProperty::set(context, V8PrivateProperty::messageEventCacheData());
> 
> V8PrivateProperty::messageEventCacheData() returns v8::Local<v8::Private>.
With
> this API, you don't need to create a Symbol object, which might decrease the
> overhead of accessing private properties (I'm not sure).
> 

In this scenario, would the private property be associated with the context, or
the MessageEvent?

WebGL uses V8HiddenValue to add properties to multiple types of objects: the JS
wrappers for WebGLRenderingContextBase, WebGLFramebuffer, WebGLProgram, etc. It
wouldn't work if it were only possible to hang private properties off of the JS
context.

[haraken, 2016-05-24 22:31:06]

On 2016/05/24 21:49:19, Ken Russell wrote:
> Thanks for the CC:.
> 
> The overall direction seems fine. v8::Private is marked an experimental API.
> Should it be taken out of experimental mode?
> 
>
https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/MessageEvent.cpp (right):
> 
>
https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/MessageEvent.cpp:228:
> V8PrivateProperty::getMessageEventCachedData(isolate).set(wrapper,
> v8String(isolate, dataAsString()));
> On 2016/05/24 17:33:53, haraken wrote:
> > 
> > Another possible API would be:
> > 
> >   V8PrivateProperty::get(context,
V8PrivateProperty::messageEventCacheData());
> >   V8PrivateProperty::set(context,
V8PrivateProperty::messageEventCacheData());
> > 
> > V8PrivateProperty::messageEventCacheData() returns v8::Local<v8::Private>.
> With
> > this API, you don't need to create a Symbol object, which might decrease the
> > overhead of accessing private properties (I'm not sure).
> > 
> 
> In this scenario, would the private property be associated with the context,
or
> the MessageEvent?
> 
> WebGL uses V8HiddenValue to add properties to multiple types of objects: the
JS
> wrappers for WebGLRenderingContextBase, WebGLFramebuffer, WebGLProgram, etc.
It
> wouldn't work if it were only possible to hang private properties off of the
JS
> context.

v8::context is just a parameter to be passed to the underlying V8 API.

> >   V8PrivateProperty::get(context,
V8PrivateProperty::messageEventCacheData());
> >   V8PrivateProperty::set(context,
V8PrivateProperty::messageEventCacheData());

V8PrivateProperty::messageEventCacheData() is the key part. The private property
is associated with MessageEvent. You can define whatever private property you
want.

[Yuki, 2016-05-26 10:52:11]

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.cpp:22: if
(!LIKELY(v8::String::NewFromOneByte(isolate, reinterpret_cast<const
uint8_t*>(symbol), v8::NewStringType::kNormal,
static_cast<int>(length)).ToLocal(&str))) {
On 2016/05/24 17:33:53, haraken wrote:
> 
> Can we use v8CallOrCrash?

Done.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:85:
v8::Local<v8::Value> getFromMainWorld(ScriptState*, ScriptWrappable*);
On 2016/05/24 17:33:53, haraken wrote:
> 
> Add a comment that this method should not be used unless you understand what
> you're doing. It's dangerous to return a main world's wrapper to another
world.

Made this one a private: member.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:94: v8::Isolate*
m_isolate;
On 2016/05/24 17:33:53, haraken wrote:
> 
> Instead of caching m_isolate, can we cache v8::Context (because Symbol should
be
> associated with v8::Context)? Then you don't need to call
> m_isolate->GetCurrentContext() in get().

v8::Private itself is NOT associated with any v8::Context.  v8::Context is
needed when we access to a v8::Object which may hold a private property.

Considering isolate->GetCurrentContext() is not that cheap, I'd like to avoid to
call it as much as possible.

Note that we support two overloaded versions of |get|, and the first version
that takes |context| is recommended.
    value = private.get(context, object);
where |context| is the context you already have.  For example,
    value = private.get(scriptState->context(), object);
is more efficient than isolate->GetCurrentContext().

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:100: return
std::unique_ptr<V8PrivateProperty>(new V8PrivateProperty());
On 2016/05/24 17:33:53, haraken wrote:
> 
> base::wrapUnique ?

Done.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:107: if
(privateProp->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName).isEmpty())
{ \
On 2016/05/24 17:33:53, haraken wrote:
> 
> Add UNLIKELY?

Done.

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/2007463003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/MessageEvent.cpp:228:
V8PrivateProperty::getMessageEventCachedData(isolate).set(wrapper,
v8String(isolate, dataAsString()));
On 2016/05/24 17:33:53, haraken wrote:
> 
> Another possible API would be:
> 
>   V8PrivateProperty::get(context, V8PrivateProperty::messageEventCacheData());
>   V8PrivateProperty::set(context, V8PrivateProperty::messageEventCacheData());
> 
> V8PrivateProperty::messageEventCacheData() returns v8::Local<v8::Private>.
With
> this API, you don't need to create a Symbol object, which might decrease the
> overhead of accessing private properties (I'm not sure).

Symbol class only holds two simple members, so the overhead should be negligible
compared to a) creating a new v8::Symbol, or b) accessing a private property
through v8::Object::GetPrivate.  Especially when it's inlined, the cost will be
zero.

Another option in my mind is to move the initialization of
ScopedPersistent<v8::Private> members in V8PrivateProperty into the constructor.
 Then, we don't need any check in getMessageEventCachedData() if we need to
create a new v8::Symbol or not.  Then, it's highly likely that everything will
be inlined and optimized.

[haraken, 2016-05-26 16:41:53]

Mostly looks good.

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:68: if
(object->GetPrivate(context, m_privateSymbol).ToLocal(&value))

Can we use v8Call?

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:72:
v8::Local<v8::Value> get(v8::Local<v8::Object> object)

Do we need the version that doesn't take the context parameter?

Ideally all call sites should always pass in the context explicitly (because
there's no guarantee that isolate->GetCurrentContext returns a correct context).

Then you can remove the m_isolate paramter from Symbol.

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp
(left):

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:74:
result = V8HiddenValue::getHiddenValue(scriptState, info.Holder(),
V8HiddenValue::stringData(info.GetIsolate()));

Why can you remove this code?

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:87:
result = V8HiddenValue::getHiddenValue(scriptState, info.Holder(),
V8HiddenValue::arrayBufferData(info.GetIsolate()));

Ditto.

[Yuki, 2016-05-27 11:49:02]

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:68: if
(object->GetPrivate(context, m_privateSymbol).ToLocal(&value))
On 2016/05/26 16:41:52, haraken wrote:
> 
> Can we use v8Call?

Done.

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:72:
v8::Local<v8::Value> get(v8::Local<v8::Object> object)
On 2016/05/26 16:41:52, haraken wrote:
> 
> Do we need the version that doesn't take the context parameter?
> 
> Ideally all call sites should always pass in the context explicitly (because
> there's no guarantee that isolate->GetCurrentContext returns a correct
context).
> 
> Then you can remove the m_isolate paramter from Symbol.

Done.

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp
(left):

https://codereview.chromium.org/2007463003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8MessageEventCustom.cpp:74:
result = V8HiddenValue::getHiddenValue(scriptState, info.Holder(),
V8HiddenValue::stringData(info.GetIsolate()));
On 2016/05/26 16:41:53, haraken wrote:
> 
> Why can you remove this code?

MessageEvent used three different hidden values before this CL; stringData,
arrayBufferData, and data.  See MessageEvent.cpp, too.  The logic was:

    In MessageEvent.cpp:
    MessageEvent* e;
    save e->dataAsString() into V8HiddenValue::stringData

    In V8MessageEventCustom.cpp:
    if (V8HiddenValue::data exists)
      return V8HiddenValue::data

    result = V8HiddenValue::stringData
    save result into V8HiddenValue::data
    return result

Why don't we directly save the value of e->dataAsString() into
V8HiddenValue::data?

I did it in this CL.  V8HiddenValue::data is renamed to
V8PrivateProperty::getMessageEventCachedData.

[haraken, 2016-05-27 18:15:20]

LGTM

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:70: if
(v8Call(object->GetPrivate(context, m_privateSymbol), value))

You were saying that you won't need to call HasPrivate before calling
GetPrivate. Do you need to call HasPrivate?

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:86:
Symbol(v8::Local<v8::Private> privateSymbol)

Add explicit.

[Yuki, 2016-05-30 06:44:28]

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:70: if
(v8Call(object->GetPrivate(context, m_privateSymbol), value))
On 2016/05/27 18:15:20, haraken wrote:
> 
> You were saying that you won't need to call HasPrivate before calling
> GetPrivate. Do you need to call HasPrivate?

I thought we don't need a call to HasPrivate, but it turned out that we actually
need this.

I thought obj->GetPrivate returns the empty handle if there is no private
property, however, it actually returns v8::Undefined.  Then, we need to
distinguish "a private property is not set" from "a private property is
intentionally set to undefined".  So, we need obj->HasPrivate() check here.

https://codereview.chromium.org/2007463003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:86:
Symbol(v8::Local<v8::Private> privateSymbol)
On 2016/05/27 18:15:20, haraken wrote:
> 
> Add explicit.

Done.

[Yuki, 2016-05-30 06:45:44]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-05-30 06:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2007463003/#ps120001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-05-30 06:45:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007463003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007463003/120001

[commit-bot: I haz the power, 2016-05-30 08:27:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-30 08:29:16]

Description was changed from

==========
binding: Reimplements V8HiddenValue as V8PrivateProperty.

V8HiddenValue is very slow.  This CL implements it again as
V8PrivateProperty so that it will be efficient.  Also makes it clear
which interface and which attribute, etc. is using which private symbol.
Note that "Class#property" is recommended as symbol names by V8.

Following CLs will replace V8HiddenValue with V8PrivateProperty gradually.

BUG=611864
==========

to

==========
binding: Reimplements V8HiddenValue as V8PrivateProperty.

V8HiddenValue is very slow.  This CL implements it again as
V8PrivateProperty so that it will be efficient.  Also makes it clear
which interface and which attribute, etc. is using which private symbol.
Note that "Class#property" is recommended as symbol names by V8.

Following CLs will replace V8HiddenValue with V8PrivateProperty gradually.

BUG=611864

Committed: https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa
Cr-Commit-Position: refs/heads/master@{#396694}
==========

[commit-bot: I haz the power, 2016-05-30 08:29:17]

Patchset 7 (id:??) landed as
https://crrev.com/4419cbd172b5d77e0e33b7622666e523d8b20afa
Cr-Commit-Position: refs/heads/master@{#396694}