1297520 - Investigate performance penalty of multilocale Firefox snaps

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Snap, defect, P3)

Description

[Rail Aliiev [:rail]]

We plan to ship multilocale Firefox snaps. This may negatively affect start-up and runtime. We should figure out what the penalty is here.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: snap.diff

We added the ability to only load one locale at a time based on the system locale.

I can't compile to test, but I think this patch should work for the snap assuming that we get the proper locale from the operating system.

Besides this change, you should package the locales into their own sub-directories, like this:

https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/repackaging/msix.py#522

Putting each locale in it's own sub-directory under distribution called locale-{localename}

Comment 2

[Olivier Tilloy]

Thanks Mike! I did a test build with your patch and the langpacks re-organized accordingly, it's currently available for testing in the candidate/one-locale channel (arm64 published, amd64 building).

It seems to work as expected. I haven't done any measurements yet, but I expect this will improve the startup time.

One thing that I have noticed though, is that when adding new locales through about:preferences, firefox seems to download the list of locales and the xpis themselves from Mozilla's servers, instead of getting them from the snap's distribution directory. Not a big deal, but perhaps existing langpacks could be used, instead of re-fetching them?

Comment 3

[Mike Kaply [:mkaply]]

Not a big deal, but perhaps existing langpacks could be used, instead of re-fetching them?

That's an interesting idea that might work in the packaged case (because the langpacks are always the latest).

I'll open a bug.

Comment 4

[Mike Kaply [:mkaply]]

Based on your comments, I'll put together an official patch and get this landed.

Comment 5

[Olivier Tilloy]

Thanks!

I have done some measurements with the test snap in the candidate/one-locale channel on a raspberry pi 4 (arm64).
Cold starts and hot starts are marginally faster than the snap in stable (by less than a second, out of a baseline of ~12s for hot starts and ~17s for cold starts).

Where I'm seeing a very noticeable improvement is for the very first cold start, without an existing profile, nor one to import from ~/.mozilla. In this case, the startup time is halved, from 34s down to 17s.

Note that these timings were recorded using a stopwatch, so they are quite imprecise, and it was done on an arm64 board where I/O is slow because the amd64 snap wasn't ready yet (it is now published).

Comment 6

[Olivier Tilloy]

Mike, can you share the links to the patch (comment 4) and the bug report (comment 3) when they're up? Thanks!

Comment 7

[Mike Kaply [:mkaply]]

Attachment: Bug 1297520 - For Snap, only install language for system locale. r?nalexander

Comment 8

[Mike Kaply [:mkaply]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1778194 is the bug for reusing locales.

Official patch is attached.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94420b6eff4c
For Snap, only install language for system locale. r=nalexander

Comment 10

[Olivier Tilloy]

Comment on attachment 9284189 [details]
Bug 1297520 - For Snap, only install language for system locale. r?nalexander

Beta/Release Uplift Approval Request

• User impact if declined: Firefox when packaged as a snap will be much slower to start the very first time it's run, because it will copy over all available langpacks to the newly created user profile.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This only affects the snap package.
• String changes made/needed:
• Is Android affected?: No

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures on test_snap.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/29fa47b9f641c69dc9f9500b2f6a5aaed486f347
Log link: https://treeherder.mozilla.org/logviewer?job_id=383516090&repo=autoland&lineNumber=3193

Comment 12

[Mike Kaply [:mkaply]]

I know this is a weird one to send your way, but I have no idea how my patch could have changed these values.

They are apparently telemetry checks that were put in with the per profile installation stuff.

Comment 13

[Donal Meehan [:dmeehan]]

Comment on attachment 9284189 [details]
Bug 1297520 - For Snap, only install language for system locale. r?nalexander

Rejecting uplift request, patch backed out from central

Comment 14

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #12)

I know this is a weird one to send your way, but I have no idea how my patch could have changed these values.

They are apparently telemetry checks that were put in with the per profile installation stuff.

The profile manager disables per-install profiles when running in snap. We test whether we are in snap by checking for the SNAP_NAME environment variable. The test simulates a snap environment by setting that variable.

But the result of getting the snap instance name is cached so I bet your patch is making that check happen on every xpcshell startup, so before the test has a chance to set the environment variable. So now the test cannot simulate running under snap and so runs with profile per install enabled.

Comment 15

[Olivier Tilloy]

That makes sense, thanks for explanation Dave.
If I understand correctly, this affects only the tests, not the actual functionality of the browser itself. Still, we need to figure out a way to make the tests simulate a snap environment reliably.

Comment 16

[Dave Townsend [:mossop]]

It may just be that we change IsRunningUnderSnap to directly check for the environment variable rather than calling the cached GetSnapInstanceName. We don't need to know the specific snap name to know we are in snap and just checking for the presence of the environment variable is likely so fast and we also check this so rarely that there isn't a need to cache the result of that.

Comment 17

[Mike Kaply [:mkaply]]

Do we still need to support snapd <= 2.35 or is something like this good enough?

bool IsRunningUnderSnap() {
  if (const auto* snapInstanceName = g_getenv("SNAP_INSTANCE_NAME")) {
   return true;
  }
  return false;
}

Comment 18

[Mike Kaply [:mkaply]]

Unfortunately it still fails with that patch

https://treeherder.mozilla.org/jobs?repo=try&revision=9d252bf71ef70b4669660e69fbc8c8522e52a5d6&selectedTaskRun=EbYl_VFCTymAj270LU66zw.0

Comment 19

[Dave Townsend [:mossop]]

(In reply to Mike Kaply [:mkaply] from comment #18)

Unfortunately it still fails with that patch

https://treeherder.mozilla.org/jobs?repo=try&revision=9d252bf71ef70b4669660e69fbc8c8522e52a5d6&selectedTaskRun=EbYl_VFCTymAj270LU66zw.0

You'll need to update the test to use SNAP_INSTANCE_NAME rather than the old SNAP_NAME

Comment 20

[Mike Kaply [:mkaply]]

(In reply to Dave Townsend [:mossop] from comment #19)

You'll need to update the test to use SNAP_INSTANCE_NAME rather than the old SNAP_NAME

Doh. All good now.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/483a0e5c3bd9
For Snap, only install language for system locale. r=nalexander,OlivierTilloy

Comment 22

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/483a0e5c3bd9