584866 - Cache LoadFrameScript Calls

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alon Zakai (:azakai)]

LoadFrameScript creates a channel and compiles the JavaScript it loads from that channel; both operations can be noticeably slow (see discussion in bug 550936).

We should cache the results here, so after we load&compile a script once, later times can use that compiled code.

Comment 1

[Alon Zakai (:azakai)]

This is needed for bug 550936, which is blocking2.0 beta4+ and blocking-fennec
2.0a1+.

Comment 2

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

Attachment: wip

This needs more testing, but passes some very simple tests without leaking
when closing the browser.

Comment 3

[Alon Zakai (:azakai)]

Tested a bit, doesn't seem to crash or leak.

I'm not sure how to test this with caching enabled, though - the current patch appears to have it disabled (scripts are still compiled once per new window). I'm not sure where to change the patch to enable caching in a simple way.

Why do we even need this as an option, btw - why not always cache scripts?

Comment 4

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

Scripts are compiled only first time and released when there are no
in-process message managers. So I guess in your txul test we might actually
compile the script for each window and release the script when the window closes.
I guess I need to use the safe JSContext to release the scripts, not the last
alive message manager context.

We do need the option not-cache, because I want that sending 
data: urls works, and we probably don't usually want to cache those.

Comment 5

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

Attachment: patch

Alon, could you try this one.
With this the cached scripts are cleared during shutdown, not when the
last frame message manager is deleted.

And when testing, just use the 3rd parameter of loadFrameScript.

Comment 6

[Alon Zakai (:azakai)]

Tested, works very well.

Only suggestion I have is to make the default to cache scripts, so we don't need to go around changing all the places that call loadFrameScript. Or maybe just not cache |data:| urls, and cache all the rest?

Comment 7

[Alon Zakai (:azakai)]

Hmm, on the try server there are some oranges about "nsBaseHashtable was not initialized properly" and some leaks about hash tables as well. Perhaps to do with the static hashtable?

Comment 8

[Alon Zakai (:azakai)]

Attachment: Patch for patch

Seems to fix the hashtable errors in the patch.

Comment 9

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

Ok, perhaps for now not caching data: and cache everything else is
good enough.

Comment 10

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

Attachment: patch

If this is ok to you Alon, I'll ask review from jst or someone.

Comment 11

[Alon Zakai (:azakai)]

Comment on attachment 463902 [details] [diff] [review]
patch

Looks great.

Comment 12

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

Jst, this is the non-e10s patch.
I'll fix e10s (content process) in a separate bug, but it will
also use nsFrameScriptExecutor.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 463902 [details] [diff] [review]
patch

+static PLDHashOperator
+EnumerateCachedScripts(const nsAString& aKey,
+                       nsFrameScriptExecutorJSObjectHolder*& aData,
+                       void* aUserArg)
+{
+  JSContext* cx = static_cast<JSContext*>(aUserArg);
+  JS_RemoveObjectRoot(cx, &(aData->mObject));

How about we rename this function to CachedScriptUnrooter() or something, since that's what it does.

r=jst

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch for checkin

This patch has jst's rename. I'll land it.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/de1fc57f30b5

Comment 16

[Alon Zakai (:azakai)]

(In reply to comment #12)
> this is the non-e10s patch.
> I'll fix e10s (content process) in a separate bug, but it will
> also use nsFrameScriptExecutor.

Posted followup bug 586115.