84074 2012-04-16 14:03:14 -0700 [meta][V8] Pass Isolate around in V8 bindings 2014-03-02 09:33:17 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 84077 84103 84161 84173 84202 84250 84257 84259 84261 84269 84271 84273 84277 84295 84299 84310 84627 84628 84637 84650 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 1 haraken haraken abarth arv dglazkov japhet krit yurys oldest_to_newest 603415 0 haraken 2012-04-16 14:03:14 -0700 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(). 603420 1 haraken 2012-04-16 14:04:10 -0700 I'll upload patches step by step. abarth@ is on vacation for two weeks, and I am happy if anyone would review them:) 604055 2 haraken 2012-04-17 08:54:53 -0700 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. 605268 3 haraken 2012-04-18 18:02:37 -0700 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). 608240 4 haraken 2012-04-23 17:40:35 -0700 (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. 624974 5 haraken 2012-05-15 22:07:53 -0700 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 630382 6 haraken 2012-05-22 08:16:30 -0700 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 631306 7 haraken 2012-05-23 00:08:08 -0700 > 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. 631628 8 haraken 2012-05-23 06:59:20 -0700 (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 631956 9 abarth 2012-05-23 13:16:06 -0700 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. 632213 10 haraken 2012-05-23 17:05:05 -0700 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. 632253 11 haraken 2012-05-23 17:51:56 -0700 (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 634993 12 haraken 2012-05-28 01:38:55 -0700 (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? 635006 13 abarth 2012-05-28 01:53:00 -0700 > (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(..) ? 635009 14 haraken 2012-05-28 01:56:31 -0700 (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. 636867 15 haraken 2012-05-29 22:46:05 -0700 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. 637446 16 abarth 2012-05-30 11:12:27 -0700 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. 638188 17 haraken 2012-05-31 03:22:19 -0700 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. 638191 18 haraken 2012-05-31 03:26:24 -0700 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. 673725 19 haraken 2012-07-19 11:27:17 -0700 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. 677891 20 haraken 2012-07-25 04:04:51 -0700 (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)