300639 - don't recompile xbl event handlers on every event dispatch

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Mike Shaver (:shaver emeritus)]

The core of this bug is simple: for every XBL-dispatched <handler> or <key>, we
recompile the handler script before dispatching.

Fixing this will involve, at the least:

1) determining exactly what behaviour is currently provided by various
combinations of oncommand/command attributes, key-vs-handler distinctions, and
dynamic updating of those;

2) caching of compiled event handler scripts, and possibly handler objects;

3) invalidation of that cache to maintain the behaviour in 1).

I have the obvious patch for 2) in my tree now, but I'm sure it doesn't handle
dynamic oncommand= or command= updating, let alone changing the oncommand= of
the element referenced in the command= attribute!

Comment 1

[Aiko]

This seems like a duplicate of bug 96977 but maybe you'd prefer to resolve the older bug instead?

Comment 2

[Mike Shaver (:shaver emeritus)]

Mass abdication.

Comment 4

[Robert Sayre]

Attachment: cache XBL event handlers

This patch shows a pretty nice hit/miss ratio. I could probably even skip a possibly expensive string equality check on handlerText by marking it dirty whenever it's set.

Open issues:

1.) The commented out portion of EnsureEventHandler shows a GC hazard I'm pretty sure, so I think I need to call nsContentUtils::HoldScriptObject, just not sure where/how.

2.) Should I bind (and thus cache) the handler in EnsureEventHandler as well? I guess it depends on whether scriptTarget will change often.

3.) Do I need to add cycle collection support?

Comment 5

[Mike Schroepfer]

Flagging with perf and adding to blocking list since this shows up in:

http://wiki.mozilla.org/Performance:FrontEnd

Comment 6

[Robert Sayre]

Attachment: don't recompile

This now successfully avoids recompiling many, many event handlers. For example, each keypress in the description field of this bug. I've left the debugging printf calls in for now.

I doubt I'm doing everything I need to regarding cycle collection, and I still want to know whether I can avoid rebinding each time. Maybe not.

Comment 7

[Robert Sayre]

From irc:

<peterv>	sayrer: so I think the nsXBLPrototypeHandler is just held by the prototype binding
<peterv>	sayrer: so just add a traverse function to nsXBLPrototypeHandler and nsCachedHandler and call those from the prototype binding's traverse
<peterv>	sayrer: since there's only ever on edge from prototype binding to nsXBLPrototypeHandler to nsCachedHandler you don't need to add nsXBLPrototypeHandler and nsCachedHandler themselves to the cycle collector

Comment 8

[Robert Sayre]

<peterv>	sayrer: also, don't use nsScriptObjectHolder
<peterv>	sayrer: I think we don't support languages other than JS in XBL, so you should use NS_HOLD_JS_OBJECTS on the nsXBLDocumentInfo that holds the nsXBLPrototypeHandler

Comment 9

[Mike Schroepfer]

Assigning a prio of 1 based on hoped perf impact and fears of risk.  Sayre please re-set if you have differing opinions.

Comment 10

[Peter Van der Beken [:peterv]]

I think you should change |nsScriptObjectHolder* mScriptObjectHolder| to |void* mEventHandler|, add |void nsXBLPrototypeHandler::Traverse(nsCycleCollectionTraversalCallback &cb) const|, |void nsXBLPrototypeHandler::Trace(TraceCallback aCallback, void *aClosure) const| and |void nsXBLPrototypeHandler::Unlink()|. Traverse needs to call NoteXPCOMChild on mCachedHandler->mGlobal and Trace needs to call aCallback on mCachedHandler->mEventHandler if it's non-null. Unlink should probably delete mCachedHandler. AFAICT that hooks it up to cycle collection. I do worry that this will keep the global and the handler alive longer than is necessary, I think nsXBLPrototypeHandlers are alive for most of the lifetime of the app. But maybe we update the cached handler often enough that that's not an issue (since they'd point to a live global anyway).

If the number of nsXBLPrototypeHandlers that we never create an nsCachedHandler for is small it might make sense to change |nsCachedHandler* mCachedHandler| to |nsCachedHandler mCachedHandler| or to allocate it as part of the nsXBLPrototypeHandler (if we know whether we'll need an nsCachedHandler before creating the nsXBLPrototypeHandler).

Do you really need to match on mHandlerText and aURL/aLineNo? I think deleting mCachedHandler when mHandlerText changes would make more sense. mLineNumber looks like it can't change, we should just pass it into the nsXBLPrototypeHandler constructor and remove SetLineNumber. Looking at the nsDocumentInfo code I don't think mPrototypeBinding->DocURI()->GetSpec(bindingURI) can change. Maybe bz or sicking know.

Comment 11

[Peter Van der Beken [:peterv]]

Looks like mEventName and onEventAtom can't change either, so Match should be able to only check the global.

Comment 12

[Mike Schroepfer]

Peter/Sayre we going to get this for b2?

Comment 13

[Damon Sicore (:damons)]

Just checked with sayre, and it's still on track for b2.

Comment 14

[Robert Sayre]

Attachment: peterv's changes

Comment 15

[Robert Sayre]

Attachment: peterv's changes

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 291517 [details] [diff] [review]
peterv's changes

> diff -r d403149ba91e content/xbl/src/nsXBLPrototypeBinding.cpp

> @@ -368,6 +374,12 @@ nsXBLPrototypeBinding::Unlink()
>  {
>    if (mImplementation)
>      mImplementation->Unlink();
> +  nsXBLPrototypeHandler* curr = mPrototypeHandler;
> +

Nit: remove this newline and add it above curr.

> +  while (curr) {

> diff -r d403149ba91e content/xbl/src/nsXBLPrototypeHandler.cpp

> +// Class used to cache handlers and avoid recompiling 
> +class nsCachedHandler
> +{

> +  nsCOMPtr<nsIScriptGlobalObject> mGlobal;
> +  void *mHandler;
> +};

I'd drop this and store mHandler and mGlobal directly in nsXBLPrototypeHandler.

> -  nsIScriptContext *boundContext = boundGlobal->GetScriptContext(stID);
> -  if (!boundContext) return NS_OK;
> -
> -  nsScriptObjectHolder handler(boundContext);
>    nsISupports *scriptTarget;
> -
>    if (winRoot) {
>      scriptTarget = boundGlobal;
>    } else {
>      scriptTarget = aTarget;
>    }
>  
> -  void *scope = boundGlobal->GetScriptGlobal(stID);
> +  nsIScriptContext *boundContext = boundGlobal->GetScriptContext(stID);
> +  if (!boundContext)
> +    return NS_OK;
> +  nsScriptObjectHolder handler(boundContext);

There's no real need for these changes, right? Leave them out so you don't get blame :-).

> +nsresult
> +nsXBLPrototypeHandler::EnsureEventHandler(nsIScriptGlobalObject* aGlobal,
> +                                          nsIAtom *aName, PRUint32 aLineNo,

Drop aLineNo and use mLineNumber.

> +                                          nsScriptObjectHolder &aHandler)
> +{
> +  nsresult rv;

Declare rv where you use it.

> +  nsIScriptContext *boundContext = 
> +    aGlobal->GetScriptContext(nsIProgrammingLanguage::JAVASCRIPT);

Should just pass this in.

> +  PRBool needsCompile = PR_TRUE;
> +  if (mCachedHandler && mCachedHandler->Match(aGlobal)) {
> +    aHandler.set(mCachedHandler->mHandler);
> +    if (!aHandler)
> +      return NS_ERROR_FAILURE;
> +    needsCompile = PR_FALSE;

Return early if you have a cached handler, probably right at the start of the function.

> +nsXBLPrototypeHandler::DispatchXBLCommand(nsPIDOMEventTarget* aTarget, nsIDOMEvent* aEvent)
> +{
> +  // This is a special-case optimization to make command handling fast.
> +   // It isn't really a part of XBL, but it helps speed things up.

Bad indentation in this function.

I'll attach a patch with the changes and check it in for beta 2 tonight.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: With peterv's additional comments

Comment 18

[Peter Van der Beken [:peterv]]

I took the liberty of checking in attachment 291613 [details] [diff] [review]. Robert: I think we can mark this fixed?

Comment 19

[Robert Sayre]

Yep.