Remove V8HiddenPropertyName

Remove V8HiddenPropertyName

V8HiddenPropertyName is used as a thin wrapper for GetHiddenValue(name) and SetHiddenValue(name) in order to prefix the name with "WebCore::HiddenProperty::". This prefix is for avoiding naming conflicts with someone who sets hidden values as well. However, given that V8 binding is the only guy who sets the hidden values on DOM wrappers, there is no need to use the prefix.

- By removing the prefix, we can remove all code about V8HiddenPropertyName.

- By removing the prefix, we can slightly improve performance of GetHiddenValue() and SetHiddenValue() since we no longer need to prefix strings nor look up V8PerIsolateData to get cached, prefixed strings. (I don't know if the performance of GetHiddenValue()/SetHiddenValue() matters though.)

Given the above, this CL removes V8HiddenPropertyName.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165735

[haraken, 2014-01-17 07:51:08]

PTAL

This CL includes https://codereview.chromium.org/140783007/, so please ignore
the MIDI part in this CL.

[kouhei (in TOK), 2014-01-17 07:56:23]

On 2014/01/17 07:51:08, haraken wrote:
> PTAL
> 
> This CL includes https://codereview.chromium.org/140783007/, so please ignore
> the MIDI part in this CL.

The plan sgtm. Just checking, but this doesn't introduce any additional
overhead, right?
I was wondering because V8HiddenPropertyName.cpp seems to be providing
v8::String cache.

[haraken, 2014-01-17 08:05:22]

On 2014/01/17 07:56:23, kouhei wrote:
> On 2014/01/17 07:51:08, haraken wrote:
> > PTAL
> > 
> > This CL includes https://codereview.chromium.org/140783007/, so please
ignore
> > the MIDI part in this CL.
> 
> The plan sgtm. Just checking, but this doesn't introduce any additional
> overhead, right?
> I was wondering because V8HiddenPropertyName.cpp seems to be providing
> v8::String cache.

The cache was needed because it was too heavy to construct prefixed names every
time. Now we don't use prefixed strings, so we don't need the cache.

However, it's true that removing the cache might regress performance. I hope
v8AtomicString() is fast enough and it will be faster than looking up the cache
on V8PerIsolateData. Let me measure performance.

[haraken, 2014-01-17 12:15:06]

On 2014/01/17 08:05:22, haraken wrote:
> On 2014/01/17 07:56:23, kouhei wrote:
> > On 2014/01/17 07:51:08, haraken wrote:
> > > PTAL
> > > 
> > > This CL includes https://codereview.chromium.org/140783007/, so please
> ignore
> > > the MIDI part in this CL.
> > 
> > The plan sgtm. Just checking, but this doesn't introduce any additional
> > overhead, right?
> > I was wondering because V8HiddenPropertyName.cpp seems to be providing
> > v8::String cache.
> 
> The cache was needed because it was too heavy to construct prefixed names
every
> time. Now we don't use prefixed strings, so we don't need the cache.
> 
> However, it's true that removing the cache might regress performance. I hope
> v8AtomicString() is fast enough and it will be faster than looking up the
cache
> on V8PerIsolateData. Let me measure performance.

I tried to write a micro-benchmark but I found that the cache was not used in
any performance-sensitive places. The cache was used only in
V8HiddenPropertyName::xxx() where xxx is one of the following attributes:

adaptorFunctionPeer
attributeListener
callback
condition
customElementAttached
customElementAttributeChanged
customElementCreated
customElementDetached	
customElementDocument	
customElementIsInterfacePrototypeObject	
customElementNamespaceURI
customElementTagName
customElementType
data
detail
document
event
error
idbCursorRequest
listener
scriptState
sleepFunction
state
toStringString
typedArrayHiddenCopyMethod
thenableHiddenPromise

I don't think these attributes are performance-sensitive.

In particular, the places that use these attributes in auto-generated code are
only V8CustomEvent.cpp, V8ErrorEvent.cpp, V8MessageEvent.cpp and
V8PopStateEvent.cpp. As far as I look at the code, none of these seems
performance-sensitive.

So PTAL.

[jochen (gone - plz use gerrit), 2014-01-17 12:29:21]

+dcarney

[dcarney, 2014-01-17 12:46:42]

On 2014/01/17 12:29:21, jochen wrote:
> +dcarney

[dcarney, 2014-01-17 12:48:20]

instead of removing the cache, it might a good idea to use v8::Private instead
and cache those.  should be more performant, and there's no naming conflict
possible

[haraken, 2014-01-17 12:54:03]

On 2014/01/17 12:48:20, dcarney wrote:
> instead of removing the cache, it might a good idea to use v8::Private instead
> and cache those.  should be more performant, and there's no naming conflict
> possible

I'm OK with using v8::Private, but the comment on v8::Private in v8.h concerns
me a bit :)

  /**
   * A private symbol
   *
   * This is an experimental feature. Use at your own risk.
   */
  class V8_EXPORT Private : public Data { ... }

Also I'm not sure if it's worth introducing cache, since
GetHiddenValue/SetHiddenValue aren't used in performance-sensitive places at the
moment.

WDYT?

[dcarney, 2014-01-17 12:56:49]

On 2014/01/17 12:54:03, haraken wrote:
> On 2014/01/17 12:48:20, dcarney wrote:
> > instead of removing the cache, it might a good idea to use v8::Private
instead
> > and cache those.  should be more performant, and there's no naming conflict
> > possible
> 
> I'm OK with using v8::Private, but the comment on v8::Private in v8.h concerns
> me a bit :)
> 
>   /**
>    * A private symbol
>    *
>    * This is an experimental feature. Use at your own risk.
>    */
>   class V8_EXPORT Private : public Data { ... }
> 
> Also I'm not sure if it's worth introducing cache, since
> GetHiddenValue/SetHiddenValue aren't used in performance-sensitive places at
the
> moment.
> 
> WDYT?

if you don't cache, you can't get back to your private symbol

ignore the comment - if we need to change the v8 api, we'll do it in step with
chrome

[abarth-chromium, 2014-01-18 08:45:47]

v8::Private > SetHiddenValue

Ideally we'd remove all callers of SetHiddenValue.  That will let V8 simplify
its object model.

[haraken, 2014-01-20 01:11:22]

+rossberg

> v8::Private > SetHiddenValue
>
> Ideally we'd remove all callers of SetHiddenValue.  That will let V8 simplify
its object model.

I'm happy to use v8::Private if the implementation is mature. The last time I
talked with rossberg, I though he was saying that v8::Private is not yet fully
optimized.

@rossberg: Will it now make sense to use v8::Private in V8 bindings instead of
SetHiddenValue/GetHiddenValue?

[rossberg1, 2014-01-20 10:46:46]

On 20 January 2014 02:11,  <haraken@chromium.org> wrote:
>> v8::Private > SetHiddenValue
>
>> Ideally we'd remove all callers of SetHiddenValue.  That will let V8
>> simplify its object model.
>
> I'm happy to use v8::Private if the implementation is mature. The last time
> I talked with rossberg, I though he was saying that v8::Private is not yet
> fully optimized.

Did I? I don't remember. I haven't done any measurements, but I would
expect private properties to be faster than hidden ones on all
accounts.

> @rossberg: Will it now make sense to use v8::Private in V8 bindings instead
> of SetHiddenValue/GetHiddenValue?

Definitely. Though the one problem we probably need to solve first is
getting some notion of global private names. Currently, they would be
per-context, which I assume is not sufficient for Blink?

/Andreas

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

[haraken, 2014-01-20 10:56:33]

> > @rossberg: Will it now make sense to use v8::Private in V8 bindings instead
> > of SetHiddenValue/GetHiddenValue?
> 
> Definitely. Though the one problem we probably need to solve first is
> getting some notion of global private names. Currently, they would be
> per-context, which I assume is not sufficient for Blink?

No, it's not sufficient. Private names need to be per-isolate. Would there be
any work-around to address the issue?

[haraken, 2014-01-20 10:57:38]

On 2014/01/20 10:56:33, haraken wrote:
> > > @rossberg: Will it now make sense to use v8::Private in V8 bindings
instead
> > > of SetHiddenValue/GetHiddenValue?
> > 
> > Definitely. Though the one problem we probably need to solve first is
> > getting some notion of global private names. Currently, they would be
> > per-context, which I assume is not sufficient for Blink?
> 
> No, it's not sufficient. Private names need to be per-isolate. Would there be
> any work-around to address the issue?

(Note: This is no urgent task, since as far as I see, there is no
performance-sensitive call path that uses SetHiddenValue/GetHiddenValue:-)

[haraken, 2014-01-24 05:59:48]

ping on this?

I guess that even if we replace GetHiddenValue/SetHiddenValue with v8::Private
at some point, this CL would be necessary as an intermediate step for the
replacement.

(1) This CL removes V8HiddenPropertyName.
(2) We can replace GetHiddenValue/SetHiddenValue with v8::Private incrementally.

So PTAL.

[abarth-chromium, 2014-01-24 06:55:25]

On 2014/01/24 05:59:48, haraken wrote:
> So PTAL.

You've listed 7 reviewers.  Who would you like to look at this CL?

[haraken, 2014-01-24 06:56:35]

On 2014/01/24 06:55:25, abarth wrote:
> On 2014/01/24 05:59:48, haraken wrote:
> > So PTAL.
> 
> You've listed 7 reviewers.  Who would you like to look at this CL?

Sorry, I should have added some of them to cc.

dcarney, jochen: PTAL.

[haraken, 2014-01-24 09:11:32]

Dan: PTAL

https://codereview.chromium.org/135843008/diff/310001/Source/bindings/v8/V8Bi...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/135843008/diff/310001/Source/bindings/v8/V8Bi...
Source/bindings/v8/V8Binding.h:690: bool
deleteHiddenValue(v8::Handle<v8::Object>, v8::Handle<v8::String>);

As Dan suggested offline, I created these helper methods. Once V8 supports
Private names, we can just replace the implementation of these methods.

(BTW, the fact that these methods don't take Isolate* implies that they are not
performance-sensitive:-)

[dcarney, 2014-01-24 09:17:28]

I'd prefer if you removed the String argument and instead passed a const char *
and an isolate, to get rid of all the atomicstring boilerplate everywhere

[haraken, 2014-01-24 09:24:52]

On 2014/01/24 09:17:28, dcarney wrote:
> I'd prefer if you removed the String argument and instead passed a const char
*
> and an isolate, to get rid of all the atomicstring boilerplate everywhere

Sometimes we already have v8::Handle<v8::String> and thus cannot get const char*
easily (See code_generator_v8.pm). Shall we make both v8::Handle<v8::String>
version and const char* version?

[dcarney, 2014-01-24 09:34:37]

On 2014/01/24 09:24:52, haraken wrote:
> On 2014/01/24 09:17:28, dcarney wrote:
> > I'd prefer if you removed the String argument and instead passed a const
char
> *
> > and an isolate, to get rid of all the atomicstring boilerplate everywhere
> 
> Sometimes we already have v8::Handle<v8::String> and thus cannot get const
char*
> easily (See code_generator_v8.pm). Shall we make both v8::Handle<v8::String>
> version and const char* version?

yeah it looks like there's only one spot where you don't have access to it, so
maybe generate a version for that function
that will anyway have to be handled specially by the Private change

[haraken, 2014-01-24 09:59:34]

On 2014/01/24 09:34:37, dcarney wrote:
> On 2014/01/24 09:24:52, haraken wrote:
> > On 2014/01/24 09:17:28, dcarney wrote:
> > > I'd prefer if you removed the String argument and instead passed a const
> char
> > *
> > > and an isolate, to get rid of all the atomicstring boilerplate everywhere
> > 
> > Sometimes we already have v8::Handle<v8::String> and thus cannot get const
> char*
> > easily (See code_generator_v8.pm). Shall we make both v8::Handle<v8::String>
> > version and const char* version?
> 
> yeah it looks like there's only one spot where you don't have access to it, so
> maybe generate a version for that function
> that will anyway have to be handled specially by the Private change

Done. PTAL.

(However, I found more than 10 places where v8::Handle<v8::String> is used. So
I'm neutral about making two versions of helper methods. That's not a big deal
anyway though:-)

[dcarney, 2014-01-24 10:06:43]

lgtm

[commit-bot: I haz the power, 2014-01-24 10:07:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/135843008/390001

[commit-bot: I haz the power, 2014-01-24 13:23:42]

Change committed as 165735

[haraken, 2014-01-30 00:46:09]

On 2014/01/24 13:23:42, I haz the power (commit-bot) wrote:
> Change committed as 165735

dan, rossberg:

Unfortunately this CL regressed performance of a couple of IndexedDB tests,
which are using SetHiddenValue/GetHiddenValue.
https://code.google.com/p/chromium/issues/detail?id=338383

I believe that if we can use v8::Private for SetHiddenValue/GetHiddenValue, the
performance issue will be gone. However, how long will it take? If it takes a
long time, it makes sense to revert this CL.

[haraken, 2014-01-31 00:03:25]

On 2014/01/30 00:46:09, haraken wrote:
> On 2014/01/24 13:23:42, I haz the power (commit-bot) wrote:
> > Change committed as 165735
> 
> dan, rossberg:
> 
> Unfortunately this CL regressed performance of a couple of IndexedDB tests,
> which are using SetHiddenValue/GetHiddenValue.
> https://code.google.com/p/chromium/issues/detail?id=338383
> 
> I believe that if we can use v8::Private for SetHiddenValue/GetHiddenValue,
the
> performance issue will be gone. However, how long will it take? If it takes a
> long time, it makes sense to revert this CL.

Ping on this? Can we use v8::Private and fix the performance issue or should we
revert the CL?

[arv1, 2014-01-31 00:09:20]

You can use v8::Private today but you would have to cache the symbol on the
blink side.

ES6 requires us to have a symbol registry that is shared between all Realms
(globals) but we do not have a solution within sight for that.


On Thu, Jan 30, 2014 at 4:03 PM, <haraken@chromium.org> wrote:

> On 2014/01/30 00:46:09, haraken wrote:
>
>> On 2014/01/24 13:23:42, I haz the power (commit-bot) wrote:
>> > Change committed as 165735
>>
>
>  dan, rossberg:
>>
>
>  Unfortunately this CL regressed performance of a couple of IndexedDB
>> tests,
>> which are using SetHiddenValue/GetHiddenValue.
>> https://code.google.com/p/chromium/issues/detail?id=338383
>>
>
>  I believe that if we can use v8::Private for
>> SetHiddenValue/GetHiddenValue,
>>
> the
>
>> performance issue will be gone. However, how long will it take? If it
>> takes a
>> long time, it makes sense to revert this CL.
>>
>
> Ping on this? Can we use v8::Private and fix the performance issue or
> should we
> revert the CL?
>
>
> https://codereview.chromium.org/135843008/
>



-- 
erik

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

[haraken, 2014-01-31 00:34:37]

> You can use v8::Private today but you would have to cache the symbol on the
> blink side.
> 
> ES6 requires us to have a symbol registry that is shared between all Realms
> (globals) but we do not have a solution within sight for that.

I see, let me try.