Open Bug 1713960 Opened 5 months ago Updated 25 days ago

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

Categories

(Core :: XPConnect, task, P2)

task

Tracking

()

REOPENED
94 Branch
Fission Milestone Future
Tracking Status
firefox-esr78 --- disabled
firefox-esr91 --- disabled
firefox91 --- disabled
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix

People

(Reporter: kmag, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fission:m95?])

Attachments

(1 file, 1 obsolete file)

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.

Fission M8.

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

Severity: -- → N/A
Fission Milestone: --- → M8
Priority: -- → P2

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

Fission Milestone: M8 → MVP

This bug is a hard blocker for Fission MVP.

Whiteboard: fission-hard-blocker

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.

Whiteboard: fission-hard-blocker → fission-soft-blocker
Assignee: kmaglione+bmo → nobody

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.

Assignee: nobody → continuation

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

Assignee: continuation → nobody
Flags: needinfo?(htsai)
Assignee: nobody → afarre
Attachment #9241777 - Attachment description: WIP: Bug 1713960 - Try to reinitialize ScriptPreloader for new sessions. → Bug 1713960 - Try to reinitialize ScriptPreloader for new sessions. r=smaug!

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.

Flags: needinfo?(htsai)
Attachment #9241777 - Attachment description: Bug 1713960 - Try to reinitialize ScriptPreloader for new sessions. r=smaug! → Bug 1713960 - Part 1: Try to reinitialize ScriptPreloader for new sessions. 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

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.

Status: NEW → ASSIGNED
Attachment #9241777 - Attachment description: Bug 1713960 - Part 1: Try to reinitialize ScriptPreloader for new sessions. r=smaug! → Bug 1713960 - Try to reinitialize ScriptPreloader for new sessions. r=smaug!
Attachment #9242204 - Attachment is obsolete: true
Attachment #9241777 - Attachment description: Bug 1713960 - Try to reinitialize ScriptPreloader for new sessions. r=smaug! → Bug 1713960 - Open script preloader cache when available. r=smaug!
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ff40f01229b
Open script preloader cache when available. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Regressions: 1733114

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.

Assignee: afarre → nobody
Status: RESOLVED → REOPENED
Flags: needinfo?(kmaglione+bmo)
Resolution: FIXED → ---

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

Fission Milestone: MVP → Future
Whiteboard: fission-soft-blocker → [fission:m95?]
You need to log in before you can comment on or make changes to this bug.