Bug 90043 - [V8] Optimize Integer::New() by caching persistent handles for small integers
Summary: [V8] Optimize Integer::New() by caching persistent handles for small integers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-26 22:12 PDT by Kentaro Hara
Modified: 2012-06-29 08:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.39 KB, patch)
2012-06-26 22:14 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (22.67 KB, patch)
2012-06-26 23:15 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (22.67 KB, patch)
2012-06-26 23:59 PDT, Kentaro Hara
haraken: commit-queue-
Details | Formatted Diff | Diff
patch for landing (23.52 KB, patch)
2012-06-28 00:34 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-06-26 22:12:22 PDT
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.
Comment 1 Kentaro Hara 2012-06-26 22:14:18 PDT
Created attachment 149677 [details]
Patch
Comment 2 Adam Barth 2012-06-26 22:46:25 PDT
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.
Comment 3 Adam Barth 2012-06-26 22:46:58 PDT
Comment on attachment 149677 [details]
Patch

Marking r- for memory leak in WebWorkers.
Comment 4 Kentaro Hara 2012-06-26 22:53:19 PDT
(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?
Comment 5 Adam Barth 2012-06-26 23:06:59 PDT
I think it is safe, but I'm not 100% sure.
Comment 6 Kentaro Hara 2012-06-26 23:08:16 PDT
CC-ing arv and antonm.
Comment 7 Kentaro Hara 2012-06-26 23:15:31 PDT
Created attachment 149687 [details]
Patch
Comment 8 Adam Barth 2012-06-26 23:26:44 PDT
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
Comment 9 Kentaro Hara 2012-06-26 23:59:06 PDT
Created attachment 149694 [details]
patch for landing
Comment 10 Kentaro Hara 2012-06-27 00:01:15 PDT
(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)?"
Comment 11 anton muhin 2012-06-27 09:24:36 PDT
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)?"
Comment 12 Erik Arvidsson 2012-06-27 13:48:36 PDT
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?
Comment 13 jochen 2012-06-27 14:45:04 PDT
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.
Comment 14 Kentaro Hara 2012-06-27 17:16:00 PDT
(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.)
Comment 15 Kentaro Hara 2012-06-27 17:21:34 PDT
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.
Comment 16 Kentaro Hara 2012-06-27 23:41:16 PDT
Comment on attachment 149694 [details]
patch for landing

A bunch of tests are crashing. Looks like we need to create a new handle.
Comment 17 Kentaro Hara 2012-06-28 00:04:35 PDT
(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...
Comment 18 Kentaro Hara 2012-06-28 00:34:15 PDT
Created attachment 149895 [details]
patch for landing
Comment 19 Kentaro Hara 2012-06-28 00:36:26 PDT
(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.
Comment 20 WebKit Review Bot 2012-06-28 01:52:14 PDT
Comment on attachment 149895 [details]
patch for landing

Clearing flags on attachment: 149895

Committed r121421: <http://trac.webkit.org/changeset/121421>
Comment 21 Yury Semikhatsky 2012-06-29 00:54:39 PDT
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.
Comment 22 Kentaro Hara 2012-06-29 00:59:04 PDT
(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...:)
Comment 23 Daniel Clifford 2012-06-29 08:29:59 PDT
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.