1713960 - Use next-session preloader cache for new processes when we had no cache at startup

Status: REOPENED

(Core :: XPConnect, task, P2)

Description

[Kris Maglione [:kmag]]

The script preloader currently only uses the cache file that was read at startup to send to new content processes, mainly for the sake of memory efficiency losses from mapping multiple versions of the cache.

When there was no cache file at startup (e.g., in new installs, new profiles, and after app updates), though, that means that new content processes have no preloader cache, and take a startup perf hit. That isn't great for Fission, and is probably part of the reason for the noticeable perf hit we see on AWFY when we start without a pre-warmed profile.

Since that's something users will actually run into (and particularly nightly power users who update often and run long sessoins), we should probably give new processes the version of the cache that we prepared for the next session when we didn't have a cache available at startup.

Comment 1

[Chris Peterson [:cpeterson]]

Fission M8.

kmag says this will be easy and will matter more for Fission than e10s.

Comment 2

[Neha Kochar [:neha]]

Affects only the first startup after update, or with a new profile. Can be done in MVP.

Comment 3

[Chris Peterson [:cpeterson]]

This bug is a hard blocker for Fission MVP.

Comment 4

[Chris Peterson [:cpeterson]]

Downgrading this bug from a Fission MVP hard blocker to a soft blocker because this bug only affects the first session after a Firefox update is applied.

Comment 5

[Chris Peterson [:cpeterson]]

Hsin-Yi says she will ask mccr8 to look at this bug while kmag is out on PTO. Hopefully we can land this in Nightly 94 before the 2021-09-30 code freeze deadline.

Comment 6

[Andrew McCreight [:mccr8]]

On further thought, I don't think I'm going to have time to work on this either this week or next week.

Comment 7

[Andreas Farre [:farre]]

Attachment: Bug 1713960 - Open script preloader cache when available. r=smaug!

Comment 8

[Andreas Farre [:farre]]

I tried the patch logging with

--- a/dom/base/nsFrameMessageManager.cpp
+++ b/dom/base/nsFrameMessageManager.cpp
@@ -1258,6 +1258,7 @@ void nsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript(
   options.setFileAndLine(url.get(), 1);
   options.setNonSyntacticScope(true);
 
+  TimeStamp then = TimeStamp::Now();
   JS::Rooted<JSScript*> script(cx);
   script =
       ScriptPreloader::GetChildSingleton().GetCachedScript(cx, options, url);
@@ -1315,6 +1316,8 @@ void nsMessageManagerScriptExecutor::TryCacheLoadAndCompileScript(
     }
   }
 
+  MOZ_DBG(TimeStamp::Now() - then);
+
   MOZ_ASSERT(script);
   aScriptp.set(script);

and then performed the same STRs:

1. ./mach run
2. wait 10 seconds
3. load aftonbladet.se in new tab (from recently visited)
4. load dn.se in new tab (from recently visited)

Only showing logs from after 10s.

Without patch:

...
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.168195 ms
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.097166 ms
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.183338 ms

With patch:

...
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.024694 ms
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.01053 ms
[MozDbg] [/home/farre/src/work/dom/base/nsFrameMessageManager.cpp:1319] TimeStamp::Now() - then = 0.047852 ms

There seems to be a clear drop here. Will push to try for some regular testing, and if it looks good I'll try to land it.

Comment 9

[Andreas Farre [:farre]]

Attachment: Bug 1713960 - Part 2: Cancel pending scripts when shutting down. r=smaug!

Part 1 of this patch series made the shutdown hang behavior of the
ScriptPreloader worse, which we solve by switching to dispatching when
we're shutting down. We also try to do less when we're we're shutting
down by canceling pending scripts instead of decoding them.

Depends on D125957

Comment 10

[Andreas Farre [:farre]]

Part 1 of this patch made the shutdown behaviour for ScriptPreloader worse as seen in https://treeherder.mozilla.org/jobs?repo=try&revision=5019b35ced389f57b2d111e36f9ac3bb03c027b7&group_state=expanded.

We fix this in part 2, that doesn't show the same problem as seen in https://treeherder.mozilla.org/jobs?repo=try&revision=834ea1e41ac95ac16976223284b2e717f21d9dc8.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ff40f01229b
Open script preloader cache when available. r=smaug

Comment 12

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3ff40f01229b

Comment 13

[Andreas Farre [:farre]]

Reverted due to regressing Base Content JS. I guess that this might be inevitable, but really don't know. Unassigning for now, :kmag do you have any idea about this.

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to Andreas Farre [:farre] from comment #13)

Reverted due to regressing Base Content JS. I guess that this might be inevitable, but really don't know. Unassigning for now, :kmag do you have any idea about this.

The first step is to go into perf herder and look at the TreeHerder links before and after the regression. Then go to the "artifacts" tab and download the two memory reports. Then you can use the diff function in about:memory to get some idea of what the issue is.

Comment 15

[Chris Peterson [:cpeterson]]

This is a memshrink bug, not a Fission bug.