Closed Bug 584866 Opened 10 years ago Closed 10 years ago

Cache LoadFrameScript Calls


(Core :: IPC, defect)

Not set



Tracking Status
blocking2.0 --- final+
fennec 2.0a1+ ---


(Reporter: azakai, Assigned: smaug)




(4 files, 1 obsolete file)

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.
Assignee: nobody → Olli.Pettay
Blocks: 550936
This is needed for bug 550936, which is blocking2.0 beta4+ and blocking-fennec
blocking2.0: --- → ?
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0a1+
Attached patch wip (obsolete) — Splinter Review
This needs more testing, but passes some very simple tests without leaking
when closing the browser.
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?
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.
Attached patch patchSplinter Review
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.
Attachment #463705 - Attachment is obsolete: true
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?
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?
Attached patch Patch for patchSplinter Review
Seems to fix the hashtable errors in the patch.
Ok, perhaps for now not caching data: and cache everything else is
good enough.
Attached patch patchSplinter Review
If this is ok to you Alon, I'll ask review from jst or someone.
Attachment #463902 - Flags: feedback?(azakai)
Comment on attachment 463902 [details] [diff] [review]

Looks great.
Attachment #463902 - Flags: feedback?(azakai) → feedback+
Attachment #463902 - Flags: review?(jst)
Jst, this is the non-e10s patch.
I'll fix e10s (content process) in a separate bug, but it will
also use nsFrameScriptExecutor.
Comment on attachment 463902 [details] [diff] [review]

+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.

Attachment #463902 - Flags: review?(jst) → review+
This patch has jst's rename. I'll land it.
Closed: 10 years ago
Resolution: --- → FIXED
(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.
blocking2.0: ? → final+
Duplicate of this bug: 543812
You need to log in before you can comment on or make changes to this bug.