Bug 84074 - [meta][V8] Pass Isolate around in V8 bindings
Summary: [meta][V8] Pass Isolate around in V8 bindings
Status: RESOLVED INVALID
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: 84650 84077 84103 84161 84173 84202 84250 84257 84259 84261 84269 84271 84273 84277 84295 84299 84310 84627 84628 84637 84656 84658 84660 84662 84663 84736 84739 84753 84757 84758 84787 84918 84919 84921 84922 84923 85022 85023 85097 85102 85205 85207 85329 86558 86566 86570 86576 86579 86744 86794 86802 86976 86977 86978 86979 86980 86981 86983 87070 87111 87193 87202 87204 87207 87209 87689 87692 87713 87807 87809 87810 87812 87814 87820 87825 87948 90238 90242 93315
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-16 14:03 PDT by Kentaro Hara
Modified: 2014-03-02 09:33 PST (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-04-16 14:03:14 PDT
Currently, V8 and V8 bindings are looking up Isolate here and there, and this is one of the main performance bottlenecks in DOM attribute getters/setters and methods.

Now V8 exposes Isolate as AccessorInfo::GetIsolate() and Arguments::GetIsolate(). By passing the Isolate around, we can remove Isolate lookups from V8 and V8 bindings.

In terms of performance, the first objective is to remove Isolate lookup from v8ExternalString() and getDOMDataStore().
Comment 1 Kentaro Hara 2012-04-16 14:04:10 PDT
I'll upload patches step by step. abarth@ is on vacation for two weeks, and I am happy if anyone would review them:)
Comment 2 Kentaro Hara 2012-04-17 08:54:53 PDT
The objective: Remove Isolate look-up from get{DOMNode,ActiveDOMNode,DOMObject,ActiveDOMObject}Map().

I'll land patches in the following steps:

[1] Add an optional Isolate argument to toV8(), toV8Slow(), wrap() and wrapSlow(). e.g. toV8(XXX* impl, Isolate* isolate = 0);
[2] Rewrite all callers of toV8(), toV8Slow(), wrap() and wrapSlow() so that they pass Isolate to these methods.
[3] Make the Isolate argument non-optional. i.e. toV8(XXX* impl, Isolate* isolate);
[4] Pass Isolate to get{DOMNode,ActiveDOMNode,DOMObject,ActiveDOMObject}Map() and remove Isolate look-up.
Comment 3 Kentaro Hara 2012-04-18 18:02:37 PDT
Progress update: I've uploaded a bunch of patches to pass Isolate to toV8(). Regarding toV8(), the remaining tasks are as follows:

(a) setDOMException() and throwError() can call toV8(). Thus we should pass Isolate to setDOMException() and throwError(). This requires a bunch of patches.

(b) SerializedScriptValue::create() can call toV8(). We should pass Isolate to SerializedScriptValue::create(). This requires a bunch of patches.

(c) Even after (a) and (b), there will be ~30 toV8()s without Isolate, but I think we can leave them as-is (unless the toV8() is a performance bottleneck). Some toV8()s are difficult to receive Isolate because they can be called from the context of WebCore (e.g. ScriptController.cpp, ScriptObject.cpp, V8NodeFilterCondition.cpp, etc).

I'll upload patches for (a) and (b).
Comment 4 Kentaro Hara 2012-04-23 17:40:35 PDT
(In reply to comment #3)
> Progress update: I've uploaded a bunch of patches to pass Isolate to toV8(). Regarding toV8(), the remaining tasks are as follows:
> 
> (a) setDOMException() and throwError() can call toV8(). Thus we should pass Isolate to setDOMException() and throwError(). This requires a bunch of patches.
> 
> (b) SerializedScriptValue::create() can call toV8(). We should pass Isolate to SerializedScriptValue::create(). This requires a bunch of patches.
> 
> (c) Even after (a) and (b), there will be ~30 toV8()s without Isolate, but I think we can leave them as-is (unless the toV8() is a performance bottleneck). Some toV8()s are difficult to receive Isolate because they can be called from the context of WebCore (e.g. ScriptController.cpp, ScriptObject.cpp, V8NodeFilterCondition.cpp, etc).
> 
> I'll upload patches for (a) and (b).

Progress update:

- setDOMException() => Uploaded almost all patches.
- throwError() => We need to refactor and fix bugs before passing Isolate around. The refactoring and bug-fixing are on-going.
- SerializedScriptValue::create() => Started.
Comment 5 Kentaro Hara 2012-05-15 22:07:53 PDT
Progress update:

- toV8(isolate) => Done
- wrap(isolate) => Done
- getDOMXXXMap(isolate) => Done
- setDOMException(isolate) => Done
- SerializedScriptValue::create(isolate) => Done
- throwError(isolate) => Started
- Pass Isolate to V8 basic values (e.g. v8::Null(), v8::Integer::New()) => Not started

Before starting the work for throwError(isolate), I think we should refactor a series of throwError().

The current world:

- throwError("message") is the same as throwError(TypeError, "message")
- throwError("message", TypeError) is the same as throwError(TypeError, "message")
- throwTypeError() is the same as throwError(TypeError, "Type error")

The refactored world:

- Basically, only throwError(TypeError, "message") is served
- For syntax sugar, throwTypeError("message") is served
Comment 6 Kentaro Hara 2012-05-22 08:16:30 PDT
Progress update:

- toV8(isolate) => Done
- wrap(isolate) => Done
- getDOMXXXMap(isolate) => Done
- setDOMException(isolate) => Done
- throwError(isolate) => Done
- SerializedScriptValue::create(isolate) => Done
- Pass Isolate to V8 basic values (e.g. v8::Null(), v8::Integer::New()) => Started
Comment 7 Kentaro Hara 2012-05-23 00:08:08 PDT
> Pass Isolate to V8 basic values (e.g. v8::Null(), v8::Integer::New()) => Started

Let me stop the work for a moment.

In Null(isolate), 'isolate' can be 0 if we cannot retrieve the Isolate (e.g. in an event handler invoked from the context of WebCore). In that case, Null(0) is called, and it crashes...

I filed a bug in V8 to make Null(0), Undefined(0), etc work correctly (http://code.google.com/p/v8/issues/detail?id=2148). After the bug is fixed in V8, I'll restart the work.
Comment 8 Kentaro Hara 2012-05-23 06:59:20 PDT
(In reply to comment #7)
> I filed a bug in V8 to make Null(0), Undefined(0), etc work correctly (http://code.google.com/p/v8/issues/detail?id=2148). After the bug is fixed in V8, I'll restart the work.

That being said, the V8 team might not want to implement the NULL check in Null(Isolate*), since it can decrease performance. More discussion here: http://code.google.com/p/v8/issues/detail?can=2&start=0&num=100&q=&colspec=ID%20Type%20Status%20Priority%20Owner%20Summary%20HW%20OS%20Area%20Stars&groupby=&sort=&id=2148
Comment 9 Adam Barth 2012-05-23 13:16:06 PDT
Eventually we will probably want to make the isolate parameter mandatory, so perhaps it's better to do the null check in an inline function on the WebKit side of the API.
Comment 10 Kentaro Hara 2012-05-23 17:05:05 PDT
Any of Null(Isolate*) patches has caused crash bugs (https://bugs.webkit.org/show_bug.cgi?id=87258). For the time being, let me roll out all Null(Isolate*) patches, i.e. r118133, r118129, r118120 and r118134.

I'll re-land them using an inline function to do the NULL check.
Comment 11 Kentaro Hara 2012-05-23 17:51:56 PDT
(In reply to comment #9)
> Eventually we will probably want to make the isolate parameter mandatory, so perhaps it's better to do the null check in an inline function on the WebKit side of the API.

Sounds reasonable. Let's implement the NULL check macro and use it for v8::Null(isolate), v8::Undefined(isolate) etc.

FYI: I've confirmed that the performance of Null(isolate) regresses by 1.6%, if V8 inserts the NULL check inside Null(isolate):
http://code.google.com/p/v8/issues/detail?id=2148#c4
Comment 12 Kentaro Hara 2012-05-28 01:38:55 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > Eventually we will probably want to make the isolate parameter mandatory, so perhaps it's better to do the null check in an inline function on the WebKit side of the API.
> 
> Sounds reasonable. Let's implement the NULL check macro and use it for v8::Null(isolate), v8::Undefined(isolate) etc.

abarth:

(1) Should we replace _all_ v8::Null(isolate)s to the macro? Or should we replace only v8::Null(isolate)s whose isolate can be 0? I would prefer the latter in terms of performance. However, the concern is that if we mix v8::Null(isolate) and the macro, people might misuse v8::Null(isolate) at the place where the macro should be used, which will cause a crash bug.

(2) What's the name of the macro? V8_NULL(isolate) is OK?
Comment 13 Adam Barth 2012-05-28 01:53:00 PDT
> (1) Should we replace _all_ v8::Null(isolate)s to the macro? Or should we replace only v8::Null(isolate)s whose isolate can be 0? I would prefer the latter in terms of performance. However, the concern is that if we mix v8::Null(isolate) and the macro, people might misuse v8::Null(isolate) at the place where the macro should be used, which will cause a crash bug.

IMHO, we should just use it where isolate can be null.  I'd like to get to the point where we never pass around a null isolate, so we can gradually remove the macro as we get better about using non-null isolates.  We just need to be careful not to cause crashes in the meantime.

> (2) What's the name of the macro? V8_NULL(isolate) is OK?

Rather than using a macro, perhaps we should use an inline function?  That should generate the same code as a macro but use better C++.  Perhaps we could call it v8Null(isolate) similar to v8String(..) ?
Comment 14 Kentaro Hara 2012-05-28 01:56:31 PDT
(In reply to comment #13)
> IMHO, we should just use it where isolate can be null.  I'd like to get to the point where we never pass around a null isolate, so we can gradually remove the macro as we get better about using non-null isolates.  We just need to be careful not to cause crashes in the meantime.

OK, makes sense.

> > (2) What's the name of the macro? V8_NULL(isolate) is OK?
> 
> Rather than using a macro, perhaps we should use an inline function?  That should generate the same code as a macro but use better C++.  Perhaps we could call it v8Null(isolate) similar to v8String(..) ?

Sure.
Comment 15 Kentaro Hara 2012-05-29 22:46:05 PDT
How should we treat v8::Undefined() and v8::Handle<v8::Value>()? Currently v8::Undefined() and v8::Handle<v8::Value>() are mixed to represent undefined.

I manually hacked generated code and measured the performance of v8::Undefined(), v8::Undefined(isolate), v8UndefinedWithCheck(isolate), and v8::Handle<v8::Value>().

// 14.6 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    return v8::Undefined();
}

// 8.4 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    return v8::Undefined(info.GetIsolate());
}

// 8.9 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    v8::Isolate* isolate = info.GetIsolate();
    return isolate ? v8::Undefined(isolate) : v8::Handle<v8::Value>();  // This simulates v8UndefinedWithCheck(isolate)
}

// 9.1 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    return v8::Handle<v8::Value>();
}


Keeping the performance in mind, I guess the best approach would be as follows:

(1) Replace all v8::Handle<v8::Value>() with v8::Undefined().
(2) Replace all v8::Undefined() with v8::Undefined(isolate) where isolate cannot be 0.
(3) Replace all v8::Undefined() with v8UndefinedWithCheck(isolate) where isolate can be 0.

Consequently, the fast path will be 8.4 ns and the slow path will be 8.9 ns. The mixture of v8::Undefined() and v8::Handle<v8::Value>() will be also resolved. WDYT?

Another approach is just to use v8::Handle<v8::Value>() everywhere. This will simplify the code most. But in this case both the fast path and the slow path will be 9.1 ns.
Comment 16 Adam Barth 2012-05-30 11:12:27 PDT
Is it worth understanding why v8::Undefined() is a different speed than v8::Handle<v8::Value>() ?  It seems like the two should be the same speed since they do the same thing.  Ideally, we'd make them be the same speed as v8::Undefined(info.GetIsolate()), which also does the same thing.
Comment 17 Kentaro Hara 2012-05-31 03:22:19 PDT
abarth:

// [A] div.xxx = 9.1 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    return v8::Handle<v8::Value>();
}

// [B] div.xxx = 9.0 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    static v8::Handle<v8::Value> v = v8::Handle<v8::Value>();
    return v;
}

// [C] div.xxx = 8.4 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    return v8::Undefined(info.GetIsolate());
}

// [D] div.xxx = 8.4 ns
v8::Handle<v8::Value> xxxAttrGetter(..., info) {
    static v8::Handle<v8::Value> v = v8::Undefined(info.GetIsolate());
    return v;
}


Comparing [A] and [B], v8::Handle<v8::Value>() itself is zero overhead. Comparing [C] and [D], v8::Undefined(info.GetIsolate()) itself is also zero overhead. Comparing [B] and [D], if we return v8::Handle<v8::Value>(), there seems to be some additional work in V8 compared to the case where we return v8::Undefined(info.GetIsolate()). Maybe V8 needs to internally generate an undefined value if we return v8::Handle<v8::Value>()??

Anyway I'll discuss it with the V8 team.
Comment 18 Kentaro Hara 2012-05-31 03:26:24 PDT
Progress update: I think that I've done 80%~ of the whole work.

What is left is v8::Undefined() and v8::Integer::New(). As discussed above, v8::Undefined() requires a discussion with the V8 team. v8::Integer::New() does not yet have an Isolate version of API. So I'll stop this work for a moment until the work is unblocked.
Comment 19 Kentaro Hara 2012-07-19 11:27:17 PDT
According to the V8 team, v8::Handle<v8::Value>() should be as fast as v8::Undefined(info.GetIsolate()). I filed a bug here: http://code.google.com/p/v8/issues/detail?id=2245

If v8::Handle<v8::Value>() becomes as fast as v8::Undefined(info.GetIsolate()), we can use v8::Handle<v8::Value>() everywhere regardless of isolate, which will make codebase much simpler.
Comment 20 Kentaro Hara 2012-07-25 04:04:51 PDT
(In reply to comment #19)
> According to the V8 team, v8::Handle<v8::Value>() should be as fast as v8::Undefined(info.GetIsolate()). I filed a bug here: http://code.google.com/p/v8/issues/detail?id=2245

The V8 team found that this performance difference is caused by branch prediction miss. Interesting. (http://code.google.com/p/v8/issues/detail?id=2245#c5)