Bug 90043 – [V8] Optimize Integer::New() by caching persistent handles for small integers

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kentaro Hara]

Just like V8BindingPerIsolateData::StringCache, we can introduce V8BindingPerIsolateData::IntegerCache that caches persistent handles for small integers. This will improve performance of Dromaeo/dom-query.html and Bindings/scroll-top.html.

[Kentaro Hara]

Created attachment 149677 [details]
Patch

[Adam Barth]

Comment on attachment 149677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149677&action=review

> Source/WebCore/bindings/v8/V8Binding.cpp:501
> +        m_smallIntegers[value] = v8::Persistent<v8::Integer>::New(v8::Integer::New(value));

Are these persistent handles leaked?  Presumably we need to Dispose them when the V8BindingPerIsolateData gets destructed.

> Source/WebCore/bindings/v8/V8Binding.h:92
> +#define NumberOfCachedSmallIntegers 64

Can we use a const rather than a #define?  Aren't compilers smart enough to use const variables for array sizes?

> Source/WebCore/bindings/v8/V8Binding.h:103
> +                return m_smallIntegers[value];

Should we return new local handles to the integers?  I guess it depends on when these handles get Disposed.

[Adam Barth]

Comment on attachment 149677 [details]
Patch

Marking r- for memory leak in WebWorkers.

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 149677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149677&action=review
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:501
> > +        m_smallIntegers[value] = v8::Persistent<v8::Integer>::New(v8::Integer::New(value));
> 
> Are these persistent handles leaked?  Presumably we need to Dispose them when the V8BindingPerIsolateData gets destructed.
> 

Will fix.

> > Source/WebCore/bindings/v8/V8Binding.h:103
> > +                return m_smallIntegers[value];
> 
> Should we return new local handles to the integers?  I guess it depends on when these handles get Disposed.

Given that the persistent handle is Disposed when the V8BindingPerIsolateData gets destructed (i.e. at the end of a Worker), would it be safe to return the same persistent handle to JavaScript?

[Adam Barth]

I think it is safe, but I'm not 100% sure.

[Kentaro Hara]

CC-ing arv and antonm.

[Kentaro Hara]

Created attachment 149687 [details]
Patch

[Adam Barth]

Comment on attachment 149687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149687&action=review

I think this is fine, but you might want to check with folks who understand Dispose better than I do.

> Source/WebCore/bindings/v8/V8Binding.h:92
> +    const int NumberOfCachedSmallIntegers = 64;

nit: NumberOfCachedSmallIntegers -> numberOfCachedSmallIntegers

[Kentaro Hara]

Created attachment 149694 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #8)
> (From update of attachment 149687 [details])
> I think this is fine, but you might want to check with folks who understand Dispose better than I do.

OK.

arv, antonm: The question is "Given that a persistent handle is Disposed when V8BindingPerIsolateData gets destructed (i.e. at the end of a Worker), would it be safe to return the same persistent handle to JavaScript (without creating a new handle every time)?"

[anton muhin]

I am sorry, I am not sure I fully understand the question.

Are you concerned that one may return some value to JS itself, then invoke ::Dispose on returned handle, and then JS will continue to use the object?

That should be alright as JS code doesn't operate on handles, but on values.

The problematic case could be something like that:

Handle<Value> val = <get cached persistent handle, w/o any new>
// Force ::Dispose on this handle
// use val

That could lead to hard to detect bugs as handle can get reused and all of sudden you'll get a different object here.

But in any event, isn't that true that V8BindingPerIsolateData are destroyed when the corresponding isolate is destroyed, or are workers a special case?

Overall, I am somewhat surprised that helps with the performance (yes, I saw your numbers, Kentaro)---handle creation should not be very expensive unless you create tons of them (what might be the case.)  And if it's a real speedup, another way to attack the problem might be with better V8 integration: those small numbers that are cached technically need no handles at all as they are Smis and shouldn't be adjusted by GC.

So, if it real buys that much, maybe V8 should provide a fast path for creation of handles for Smis.

I may be biased here though as I considered this optimization myself, but decided it's not worth it :)

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 149687 [details] [details])
> > I think this is fine, but you might want to check with folks who understand Dispose better than I do.
> 
> OK.
> 
> arv, antonm: The question is "Given that a persistent handle is Disposed when V8BindingPerIsolateData gets destructed (i.e. at the end of a Worker), would it be safe to return the same persistent handle to JavaScript (without creating a new handle every time)?"

[Erik Arvidsson]

This makes me sad. It is very unfortunate that we need to do this. Could we get V8 to be faster for the greater good?

I agree that this should probably be adressed on the V8 side. Is there a V8 bug about this?

Also, if you can't guarantee that the persistent handle you have is the last handle to the object, you shouldn't dispose it. Alternatively, you could make the handle weak, so the object gets collected when it's no longer needed.

[Kentaro Hara]

(In reply to comment #12)
> This makes me sad. It is very unfortunate that we need to do this. Could we get V8 to be faster for the greater good?

I am sad too, but I think that at some point the persistent handles should be cached in the V8 binding side. v8::Integer::New() is already caching objects for small integers, but v8::Integer::New() cannot avoid creating a local handle for the cached object every time, in terms of providing a general V8 API. Thus it would be a work of the V8 binding side to appropriately manage the lifetime of the persistent handles, depending on the lifetime of Workers etc. (The same discussion holds for V8BindingPerIsolateData::StringCache.)

[Kentaro Hara]

Thanks, antonm!

(In reply to comment #11)
> The problematic case could be something like that:
> 
> Handle<Value> val = <get cached persistent handle, w/o any new>
> // Force ::Dispose on this handle
> // use val
> 
> That could lead to hard to detect bugs as handle can get reused and all of sudden you'll get a different object here.

This is the exact problem I am concerning about.

> But in any event, isn't that true that V8BindingPerIsolateData are destroyed when the corresponding isolate is destroyed, or are workers a special case?

Yes, I and abarth think that it is true (i.e. the patch won't cause the problem). We wanted to confirm that the above problem won't happen both in the main thread and in workers.

[Kentaro Hara]

Comment on attachment 149694 [details]
patch for landing

A bunch of tests are crashing. Looks like we need to create a new handle.

[Kentaro Hara]

(In reply to comment #16)
> (From update of attachment 149694 [details])
> A bunch of tests are crashing. Looks like we need to create a new handle.

It seems that this is not a problem of whether we create a new local handle or not. m_smallIntegers[i].Dispose() in ~IntegerCache() crashes. Investigating...

[Kentaro Hara]

Created attachment 149895 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #17)
> It seems that this is not a problem of whether we create a new local handle or not. m_smallIntegers[i].Dispose() in ~IntegerCache() crashes. Investigating...

Just swapped the order of isolate->Exit() and ~V8BindingPerIsolateData(). ~V8BindingPerIsolateData() should be called before isolate->Exit(), because ~V8BindingPerIsolateData() can call V8 APIs (Dispose() in our case) that require the current isolate.

[WebKit Review Bot]

Comment on attachment 149895 [details]
patch for landing

Clearing flags on attachment: 149895

Committed r121421: <http://trac.webkit.org/changeset/121421>

[Yury Semikhatsky]

Comment on attachment 149687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149687&action=review

> Source/WebCore/bindings/v8/V8Binding.h:105
> +            return v8::Integer::New(value);

You might want to change v8::Integer::New signature to accept isolate as an optional second parameter.

[Kentaro Hara]

(In reply to comment #21)
> > +            return v8::Integer::New(value);
> 
> You might want to change v8::Integer::New signature to accept isolate as an optional second parameter.

I've been asking the V8 team to accept isolate for three months...:)

[Daniel Clifford]

and we've been listening. :-) It's on our list of things to tackle next, as you know we were recently focusing on adding other isolate parameter support to the first round of hot APIs you identified.