[meta][V8] Pass Isolate around in V8 bindings

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

[Kentaro Hara]

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().

[Kentaro Hara]

I'll upload patches step by step. abarth@ is on vacation for two weeks, and I am happy if anyone would review them:)

[Kentaro Hara]

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.

[Kentaro Hara]

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).

[Kentaro Hara]

(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.

[Kentaro Hara]

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

[Kentaro Hara]

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

[Kentaro Hara]

> 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.

[Kentaro Hara]

(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

[Adam Barth]

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.

[Kentaro Hara]

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.

[Kentaro Hara]

(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

[Kentaro Hara]

(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?

[Adam Barth]

> (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(..) ?

[Kentaro Hara]

(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.

[Kentaro Hara]

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.

[Adam Barth]

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.

[Kentaro Hara]

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.

[Kentaro Hara]

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.

[Kentaro Hara]

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.

[Kentaro Hara]

(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)