1616400 - Every process on Android decompressees omni.ja twice, second decompression takes 78ms on Moto G5

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Markus Stange [:mstange]]

Here's a profile from content process startup in Fenix: https://perfht.ml/2HBcE4Y

You can see two instances where we're spending 240ms in nsZipCursor::ReadOrCopy: One in mozilla::Omnijar::InitOne, and one in nsJARChannel::Open when we read the first JS file. Are the two reading the same file?

Comment 1

[Florian Quèze [:florian]]

We have this twice in the content process, but also twice in the parent, and once in the socket process: https://perfht.ml/2V6pLmo
Could we add a label frame indicating the name of the zip file we are inflating? nsZipHandle::Init seems to be the first stack frame in common between the 2 paths.

Before looking at the profile, I was wondering if this bug could be due to bug 1376994 (on desktop for historical reasons we have 2 omni.ja files that are both opened at startup).

Now I wonder if we might have omni.ja optimizations that are bypassed because of urls containing the double jar: prefix. (Ie. the first JS XPCOM component we load has this url: jar:jar:file:///data/app/org.mozilla.fenix.debug-JE9v9h4VqJa-teJvmpnKHA==/base.apk!/assets/omni.ja!/components/GeckoViewStartup.js)

The second opening of the omni.ja file on the parent process seems to be triggered by loading resource:///modules/ContentObservers.js as a process script (by the way, this seems to be the first file loaded from resource:/// (ie. would be in browser/omni.ja on desktop) rather than resource://gre/ (platform omni.ja file)).
On the content process, it's when importing resource://gre/modules/ContentProcessSingleton.jsm as a JS module.

Comment 2

[Markus Stange [:mstange]]

(In reply to Florian Quèze [:florian] from comment #1)

Could we add a label frame indicating the name of the zip file we are inflating? nsZipHandle::Init seems to be the first stack frame in common between the 2 paths.

Good idea. I've attached a patch to bug 1616705, and the label confirms that it's assets/omni.ja in both calls.

Comment 3

[Markus Stange [:mstange]]

I've been reading through the patches for bug 1265621 and the surrounding code and will investigate some more.

Comment 4

[Markus Stange [:mstange]]

I think I have a fix!
Before: https://perfht.ml/39Z4LSQ
After: https://perfht.ml/2PiVugn

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1616400 - Do not recreate the inner reader for nested omni.ja archives if the Omnijar already has it. r=glandium, r=froydnj

This is already done for the outer nsJAR, but it wasn't done for the inner nsJAR.
(The omnijar is a nested zip archive on Android: the outer archive is the APK and the inner is the omnijar file.)
So we were re-using the file mapping but not the result of the uncompression.

Comment 6

[Markus Stange [:mstange]]

New profiles regular / without stack sampling overhead, focused on content process startup:
Before: https://perfht.ml/3c87LhB / https://perfht.ml/3c4dvJg
After: https://perfht.ml/3caTxMT / https://perfht.ml/2VkEREX

In the "no stack sampling" profiles, the marker for the first "ChromeUtils.import" in the content process go from 97ms to 19ms, so that's a 78ms saving in the content process. Same 78ms saved in the parent process: The first "JS XPCOM" marker goes from 111ms -> 33ms.

Same profiles but focused on parent process:
Before: https://perfht.ml/3c7X4fa / https://perfht.ml/3a05rHu
After: https://perfht.ml/395zSMI / https://perfht.ml/3a5RxUw

Comment 7

[Markus Stange [:mstange]]

Current status: I want to do the following things before putting this patch up for review:

• Add a comment to the new method
• Push to try
• Understand the lifetime management of the nested nsJAR a bit better, and whether CloseArchive on nested archives even does anything

Comment 8

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #7)

Current status: I want to do the following things before putting this patch up for review:

• Add a comment to the new method

done

• Push to try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29cf8a50ad90efe4f26991612038eb4271269acb

• Understand the lifetime management of the nested nsJAR a bit better, and whether CloseArchive on nested archives even does anything

Yes, CloseArchive makes the (shared) nsZipArchive drop its pointer to the zip handle and clear its arena. We can only do this to an nsZipArchive that this nsJAR is the sole owner of. We need to avoid doing it for the shared omnijar archive.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a4540217d4a
Do not recreate the inner reader for nested omni.ja archives if the Omnijar already has it. r=aklotz

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8a4540217d4a