Stop reading empty chrome.manifest files in app/gre dirs unless -app is passed or we're running a non-omni.ja build

RESOLVED FIXED in Firefox 68

Status

()

task
RESOLVED FIXED
2 months ago
Last month

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug, {main-thread-io, perf})

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [fxperf:p2] [fxperfsize:S])

Attachments

(1 attachment)

Assignee

Description

2 months ago

We currently read chrome.manifest from both the app and the gre dir early on startup, from the component manager's RereadChromeManifests. These list JS components and some category manager registrations. We should really have only 1 omni.ja + manifest, which is bug 1376994.

However, I wonder if we can avoid this access entirely by compiling in the list of components/manifests on non-local/artifact builds, like we did for xpt file info.

If that's hard, I wonder if we could avoid paying some of the IO cost here by moving the file(s) into omni.ja(s) .

Andrew, do you have any idea if either of these options are feasible, or what would block them (asking because AFAICT you did some of the XPT work) ?

Flags: needinfo?(continuation)

Outside of Android, we don't have many remaining component or category registrations in chrome.manifest files.

Making resource and chrome content/skin/override entries static is easy enough. The main problem is locale entries, since the way that we generate them is super intricate, and the repacker code is... both unfortunate and incapable of modifying binaries.

It looks like Kris answered this, better than I could have. :)

Flags: needinfo?(continuation)

Also, I'm not convinced that reading these files is a significant performance concern. We probably spend a couple of milliseconds parsing them and building hashtables when they're large enough, but reading them, it shouldn't matter whether they're one file or many.

The whole omni.ja file is memory mapped, and files that we read during startup are part of a read-ahead block. They'll already be in memory when we try to access them. The only real overhead comes from looking them up in the jar index.

The real overhead comes from just opening the omni.ja files, parsing the index, and doing the read-ahead. All of that is going to happen no matter what we do with the chrome manifest files.

(In reply to Kris Maglione [:kmag] from comment #1)

Making resource and chrome content/skin/override entries static is easy enough. The main problem is locale entries, since the way that we generate them is super intricate, and the repacker code is... both unfortunate and incapable of modifying binaries.

It looks like you are talking here about the manifest files that are within the omni.ja files. These files shouldn't matter much, for the reasons you explain in comment 3.

But I'm under the impression that this bug is about the 0 bytes chrome.manifest files that live outside the omni.ja files. I wonder if we could skip reading them when we managed to open omni.ja (I'm hoping this information is somehow available without doing I/O). And also skip packaging them.

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

But I'm under the impression that this bug is about the 0 bytes chrome.manifest files that live outside the omni.ja files. I wonder if we could skip reading them when we managed to open omni.ja (I'm hoping this information is somehow available without doing I/O). And also skip packaging them.

Oh, yeah, those need to die by fire. I think that would make some enterprise people cranky, though.

Type: defect → task

Back when the empty files were added, comm-central still had binary xpcom components, and needed non-empty files to register them. Now that the component manager doesn't even support "binary-component" manifest entries, comm-central doesn't need a non-empty chrome.manifest. So yes, it can be skipped now, except as already mentioned, I'm sure there are enterprise people relying on the "feature".

FWIW, in the packager, the relevant code to change should essentially be to revert https://hg.mozilla.org/mozilla-central/rev/ccf231e054b5

Not reading chrome.manifest if one was read from omni.ja shouldn't be difficult. But a fallback to read one if omni.ja was not read is still necessary for e.g. local builds.

Assignee

Comment 7

2 months ago

(In reply to Kris Maglione [:kmag] from comment #5)

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

But I'm under the impression that this bug is about the 0 bytes chrome.manifest files that live outside the omni.ja files. I wonder if we could skip reading them when we managed to open omni.ja (I'm hoping this information is somehow available without doing I/O). And also skip packaging them.

Oh, yeah, those need to die by fire. I think that would make some enterprise people cranky, though.

Mike, can you elaborate on this?

Flags: needinfo?(mozilla)

Oh, yeah, those need to die by fire. I think that would make some enterprise people cranky, though.

Actually, that wasn't done for enterprise reasons. That was done because we were reading the files anyway, and chrome.manifest files could be dropped into the app directory to affect Firefox. By making the file already exist, it got overwritten on a Firefox update if it was ever modified.

But again, the chrome.manifest in the appdir needs to be read to support application.ini apps.

To Kris's point though, do we have any evidence that these are actually impacting startup performance in a major way? At some point there are diminishing returns here.

I realize we're making major changes in other parts of Firefox, but in a lot of these cases, we're changing use patterns that have been around for 15+ years. That shouldn't be done without more data.

Flags: needinfo?(mozilla)

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

But again, the chrome.manifest in the appdir needs to be read to support application.ini apps.

So only if the -app flag exists?

do we have any evidence that these are actually impacting startup performance in a major way?

Yes. It even impacts content process startup, as we read these files there too.

So only if the -app flag exists?

Yep. We can put a lot of this around -app.

Assignee

Comment 11

2 months ago

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

do we have any evidence that these are actually impacting startup performance

Yes, mainthread IO impacts startup performance. See the tracker bugs ( https://bugzilla.mozilla.org/showdependencytree.cgi?id=1543096 ) for how much mostly-pointless IO we're doing -- and not everything is on file yet. Sure, most of the individual files are small, and on our fast machines (mostly using SSDs) the impact of the individual files may seem to be minimal, but the cumulative effect of all this IO is substantial, especially on spinny disk machines, and needs addressing. It's not really productive to relitigate this for every individual piece of IO.

(In reply to :Gijs (he/him) from comment #11)

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

do we have any evidence that these are actually impacting startup performance

It's not really productive to relitigate this for every individual piece of IO.

My impression of this question is "do we have any evidence that this is impacting startup performance to the point of necessitating that we make moderately significant changes in firefox". Or "do the larger changes that we have to make give enough of a win to justify making them in the first place". And that seems like a fair question to ask. Because the above comments don't contain any evidence aside from "this does main thread I/O".

Assignee

Comment 13

2 months ago

(In reply to Nathan Froyd [:froydnj] from comment #12)

My impression of this question is "do we have any evidence that this is impacting startup performance to the point of necessitating that we make moderately significant changes in firefox". Or "do the larger changes that we have to make give enough of a win to justify making them in the first place". And that seems like a fair question to ask. Because the above comments don't contain any evidence aside from "this does main thread I/O".

My understanding is that just not reading these files as part of normal Firefox startup isn't actually a significant change. The comments so far indicate that we can just do so today as long as we make an exception for runs with -app and local builds without omni jar. Am I missing something?

Gathering data here (and likewise for all the other similar bugs along the lines of "let's just read this one small file real quick") would be a significant task that sets us back several releases. I don't think doing that makes sense.

Flags: needinfo?(nfroyd)
Assignee

Updated

2 months ago
Summary: Consider if we can avoid chrome.manifest file access in app/gre dirs → Stop reading empty chrome.manifest files in app/gre dirs unless -app is passed or we're running a non-omni.ja build

(In reply to :Gijs (he/him) from comment #13)

(In reply to Nathan Froyd [:froydnj] from comment #12)

My impression of this question is "do we have any evidence that this is impacting startup performance to the point of necessitating that we make moderately significant changes in firefox". Or "do the larger changes that we have to make give enough of a win to justify making them in the first place". And that seems like a fair question to ask. Because the above comments don't contain any evidence aside from "this does main thread I/O".

My understanding is that just not reading these files as part of normal Firefox startup isn't actually a significant change. The comments so far indicate that we can just do so today as long as we make an exception for runs with -app and local builds without omni jar. Am I missing something?

That is my understanding of comment 10 and the previous discussion, yes. But other people are much more knowledgeable about how all the pieces here fit together than I am.

Flags: needinfo?(nfroyd)
Assignee

Comment 15

2 months ago

Based on the discussion so far, I believe that not reading these files when -app is not passed is going to be a relatively straightforward win, so marking up priority/effort accordingly.

Whiteboard: [fxperf] → [fxperf:p2] [fxperfsize:S]
Assignee

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Let's take a step back. What are we trying to keep supporting here? The only legitimate reason I can think of for a chrome.manifest to exist outside of an omni.ja, at this point, is when an omni.ja doesn't exist at all. Things that run with -app, do have an omni.ja and a chrome.manifest... well those, if they exist, can "just" modify their app to not have that chrome.manifest (or not use omni.ja).

So it would seem to me we could approach this by reading chrome.manifest from the omni.ja if there's one, and from the file system if there isn't.

WDYT?

Assignee

Comment 18

2 months ago

(In reply to Mike Hommey [:glandium] from comment #17)

Let's take a step back. What are we trying to keep supporting here? The only legitimate reason I can think of for a chrome.manifest to exist outside of an omni.ja, at this point, is when an omni.ja doesn't exist at all. Things that run with -app, do have an omni.ja and a chrome.manifest... well those, if they exist, can "just" modify their app to not have that chrome.manifest (or not use omni.ja).

So it would seem to me we could approach this by reading chrome.manifest from the omni.ja if there's one, and from the file system if there isn't.

WDYT?

So, I'm fine with this but:

  1. I don't know much about today's consumers of -app, so I'm probably not the best person to make this call. Maybe mkaply can help us make a decision here.
  2. I suspect we'll need the "did we get passed -app" info anyway for bug 1543746 (though perhaps we can do the same there?). That said, basing the reading of these files on the presence/absence of omni.ja would also fix these reads in the content process so that would be a cleaner solution to the entire problem.
Flags: needinfo?(mozilla)

Totally agree with glandium. Don't know why we didn't think of that. If there is an omni.ja, don't read chrome.manifest.

As far as the defaults/prefs files go, I think we can take the same path.

Use the presence of omni.ja as the "you're Firefox or Thunderbird"

Flags: needinfo?(mozilla)
Assignee

Comment 20

2 months ago

Alright, I've marked as 'plan changes' to take this out of glandium's queue, and will put up a new version that does what we discussed sometime in the near future, but my attention for this week has been forced elsewhere so it might take a bit.

Attachment #9061528 - Attachment description: Bug 1543761 - stop reading chrome.manifest files in the app/gre dirs in the parent process when no -app switch has been passed, r?glandium → Bug 1543761 - stop reading chrome.manifest files in the app/gre dirs when using omni.ja, r?glandium

Comment 21

Last month
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/55d8ca64343b
stop reading chrome.manifest files in the app/gre dirs when using omni.ja, r=glandium

Comment 22

Last month
bugherder
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.