All users were logged out of Bugzilla on October 13th, 2018

Try to avoid XPCCallContexts in quickstubs for wrapper-cached things

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: bzbarsky, Assigned: peterv)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 388298 [details] [diff] [review]
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).
Assignee: nobody → peterv
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
Created attachment 392307 [details] [diff] [review]
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.
Attachment #388298 - Attachment is obsolete: true
Attachment #392307 - Flags: superreview?(jst)
Attachment #392307 - Flags: review?(jst)
(Reporter)

Comment 3

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

Updated

9 years ago
Attachment #392307 - Flags: superreview?(jst)
Attachment #392307 - Flags: superreview+
Attachment #392307 - Flags: review?(jst)
Attachment #392307 - Flags: review+
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
(Assignee)

Comment 5

9 years ago
Created attachment 393785 [details] [diff] [review]
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.
Attachment #392307 - Attachment is obsolete: true
Attachment #393785 - Flags: superreview+
Attachment #393785 - Flags: review+
(Assignee)

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/f67bf1318100
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.