840201 - Optimize calling WebIDL callbacks

Status: NEW

(Core :: DOM: Bindings (WebIDL), defect, P5)

Description

[Boris Zbarsky [:bzbarsky]]

It manages to be slower than an XPCWrappedJS.

There's a bunch of stuff going on here, but the highlights are:

976.2ms   35.9%	22.9	 	         mozilla::dom::CallbackObject::CallSetup::CallSetup(JSObject*, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling)
557.9ms   20.5%	5.3	 	          nsScriptSecurityManager::CheckFunctionAccess(JSContext*, void*, void*)
193.7ms    7.1%	6.4	 	          nsCxPusher::Push(JSContext*, bool)
103.2ms    3.7%	9.0	 	          nsJSUtils::GetStaticScriptGlobal(JSObject*)
472.0ms   17.3%	5.2	 	          JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*)
412.0ms   15.1%	0.9	 	          mozilla::dom::XPCOMObjectToJsval(JSContext*, JSObject*, xpcObjectHelper&, nsID const*, bool, JS::Value*)
215.5ms    7.9%	8.0	 	         mozilla::dom::CallbackObject::CallSetup::~CallSetup()
96.0ms    3.5%	5.6	 	          nsCxPusher::Pop()
47.7ms    1.7%	4.0	 	          nsJSContext::ScriptEvaluated(bool)
184.8ms    6.8%	1.6	 	         mozilla::dom::XPCOMObjectToJsval(JSContext*, JSObject*, xpcObjectHelper&, nsID const*, bool, JS::Value*)
40.5ms    1.4%	0.6	 	         JS_WrapObject(JSContext*, JSObject**)

So converting the event and event target to jsvals, the nsCxPusher stupidity, and the "is script enabled" check.  This last is worst of all.

We probably need several bugs blocking this one covering those issues...  As things stand, converting event listeners to this stuff doesn't seem ok.

Comment 1

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #0)
> We probably need several bugs blocking this one covering those issues...  As
> things stand, converting event listeners to this stuff doesn't seem ok.

Yes. And maybe this will provide extra incentive to get rid of all the CheckFunctionAccess crap. :-)

Comment 2

[Boris Zbarsky [:bzbarsky]]

We need to get rid of that anyway, sure but the slow part is all the work we need to tell whether script is enabled for a particular "context".

Really, whether script is enabled is a per-global state. Can we just store it on the global somewhere, or in some sort of cache keyed on the global?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Really, whether script is enabled is a per-global state. Can we just store
> it on the global somewhere, or in some sort of cache keyed on the global?

SGTM. Given that disabled script is the uncommon case, we can just store a cache of globals where the script is disabled.

Can script be disabled for arbitrary globals, or only for DOM windows? I'm presuming the latter, because that's the only thing that makes sense if we currently store it on the nsIScriptContext. We can start by making it a faster getter on nsPIDOMWindow, and then implement the cache if it's still hot?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can script be disabled for arbitrary globals, or only for DOM windows? 

That's a good question.  I guess right now running on a JSContext with no nsIScriptContext allows execution, indeed, so just windows.

> We can start by making it a faster getter on nsPIDOMWindow

You can't make it faster without a cache.  See what nsScriptSecurityManager::CanExecuteScripts actually ends up doing; the getter would have to duplicate most of it...

We should probably move this discussion to bug 840488.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Testcase for this is http://mozilla.pettay.fi/moztests/events/event_speed_2.1.html maybe with the depth set to 1.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Or perhaps http://mozilla.pettay.fi/moztests/events/event_speed_3.html

Comment 7

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I just remeasured and now we're no worse than XPCWrappedJS.  We can still do way better here, of course....

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given comment 7, does this still block 862627? I guess that is the case given that the dependency was added after comment 7.

So who can own this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't think this blocks flipping the switch in bug 862627.  I just wanted us to not lose track of this bug.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Changing the summary a bit, since 'ghastly' is a bit too strong.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Changing the summary a bit, since 'ghastly' is a bit too strong.

But ghastly was a perfect keyword to use in the awesomebar to find this bug! ;-)

/me tags his bookmark as such.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So bug 840488 will help a good bit.  After that the remaining hotspots will have to do with the cxpusher and all the slow (QI!) stuff in nsJSUtils::GetStaticScriptGlobal.

Comment 13

[Boris Zbarsky [:bzbarsky]]

This probably needs to be re-measured, since cxpusher is long-gone and so is nsJSUtils::GetStaticScriptGlobal.