499199 - Try to avoid XPCCallContexts in quickstubs for wrapper-cached things

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Profiling the domGetElements test in the peacekeeper benchmark, we seem to spend a good bit of our time in XPCCallContext creation and converting natives to jsobjects via those XPCCallContexts.  I was wondering whether we could get away with not doing any of that when the return value is wrapper-cached.  For example, for getElementsByTagName or getElementById...

That said, I haven't looked at the benchmark that carefully; it's possible that they're getting different elements/nodelists, in which case we'd still have to go and create the wrappers.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: WIP

This seems to help a bit with Dromaeo. Though I don't really like how I pass the arguments for the XPCCallContext constructor down to where it's constructed. I also worked a bit on a patch to allow constructing a slim wrapper without a XPCCallContext, but that's a bit harder (almost all functions in XPConnect get various state from the XPCCallContext).

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Introduces a new XPCLazyCallContext which holds the things we need to create an XPCCallContext. It'll create a XPCCallContext the first time we call GetXPCCallContext. XPCLazyCallContext can also wrap an existing XPCCallContext, so we can call certain functions with or without an existing XPCCallContext. XPCLazyCallContext can also hand out its JSContext, it'll call JS_BeginRequest if needed.

The quickstubs code will use XPCLazyCallContext instead of XPCCallContext when possible. For xpc_qsXPCOMObjectToJsval it currently uses an XPCCallContext to keep the XPCInterface alive during the call to xpc_qsXPCOMObjectToJsval, I've changed that so that it passes the IID and the pointer to the XPCInterface cache. In NativeInterface2JSObject we use an AutoMarkingNativeInterfacePtr on the cached pointer, then if it's non-null we just use it or we reget the XPCInterface for that IID and store it in the cache (the cache is cleared when XPConnect shuts down). That allows us to still safely use the cache without needing an XPCCallContext in the caller. I've made xpc_qsUnwrapThisImpl call XPCWrappedNative::GetWrappedNativeOfJSObject (so we can store the wrapper in the XPCLazyCallContext), it might be slightly slower but I didn't see it show up in profiles. As a bonus, QS and non-QS methods share code and they will behave the same now.

XPC_WN_Helper_NewResolve is a method that's called a lot and in which we create an XPCCallContext. We only need that if ALLOW_PROP_MODS_DURING_RESOLVE is set or ALLOW_PROP_MODS_TO_PROTOTYPE is not set. ALLOW_PROP_MODS_DURING_RESOLVE was set for DOM objects, but looking at PopulateJSClass WANT_*_PROPERTY and USE_JSSTUB_FOR_*PROPERTY override that, and all DOM objects have one or the other of those flags so we can just remove ALLOW_PROP_MODS_DURING_RESOLVE. Only Location doesn't have ALLOW_PROP_MODS_TO_PROTOTYPE. So with these changes we don't need an XPCCallContext for most of the NewResolve calls.

This gives about 5% improvement on the Dromaeo DOM tests.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This gives a 10%+ boost on some microbenchmarks (e.g. the one I just attached to bug 507683).

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 392307 [details] [diff] [review]
v1

+    XPCLazyCallContext(XPCContext::LangType callerLanguage, JSContext* cx,
+                       JSObject* obj = nsnull,
+                       JSObject* currentJSObject = nsnull,
+                       XPCWrappedNative* wrapper = nsnull,
+                       XPCWrappedNativeTearOff*tearoff = nsnull)

Add a space before tearoff here?

- In XPC_WN_Helper_NewResolve():
+        XPCPerThreadData* tls;
+        if(!allowPropModsToPrototype || allowPropModsDuringResolve)
+        {
+            XPCCallContext& ccx = lccx.GetXPCCallContext();
+            if(!ccx.IsValid())
+                return Throw(NS_ERROR_FAILURE, cx);
+
+            tls = ccx.GetThreadData();

Could we avoid creating the XPCCallContext here and use XPCPerThreadData::GetData(cx) instead (which is way fast on the main thread)? Same thing in a couple of more places in this method...

r+sr=jst

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Turns out we have to keep the XPCCallContext in NewResolve and in APIs like WrapNative, because it pushes a context on the context stack. So this patch only uses the lazy call context for quickstubs (so it's essentially a subset of the reviewed patch). We can probably still use a lazy call context for WrapNativeToJSVal, but I'll file a separate bug about that.

Comment 6

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/f67bf1318100