1117858 - extensions.xpiState is too large for a preference

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Gummi]

During startup, and periodically after that, I get this in the js console:

> Warning: attempting to write 35858 bytes to preference sites.
> This is bad for general performance and memory usage.
> Such an amount of data should rather be written to an external file.

Almost all of it is from the extensions.xpiState preference.
That pref contains a json document.
It was introduced in https://hg.mozilla.org/mozilla-central/rev/adb01d557168 (bug 1049142).
It should be stored outside, in extensions.json, extensions.sqlite, or another external file.

Comment 1

[:Gijs (he/him)]

Dave, can you comment as to how serious this is and/or how to prio? :-)

Comment 2

[Dave Townsend [:mossop]]

The warning is complaining that storing this much data in prefs can slow down startup because the prefs file is read synchronously. The problem is that we need that data synchronously during startup. We could move it into its own standalone file but we'd still have to read that data synchronously so the amount of time to read the data would remain the same but we'd be adding file stats and opens so would probably only make startup time slower.

We could look into using a more efficient way of storing the same data, at the very least as pretty repetitive text gzip would probably reduce the size needed to store by quite a bit, but then you add the decompression cost. I'd be interested to see the cost of that weight against the saved I/O cost. Since the size of the data scales with the number of extensions to install I'd expect adding a decompression stage to be harmful to the average user.

So, I don't think this is too serious or a high priority.

Comment 3

[Dave Garrett]

The size of this pref could be greatly reduced if full paths for all addons weren't stored in here. There's A LOT of repetition from doing this. (includes OS user home folder path) For example, in my main profile I have 17 enabled addons and 27 disabled addons. The pref value is currently 7.8KB. Just adding up the total size from the path to my profile's extensions folder is about 2KB. Additionally, every addon has its ID listed and file/folder name, yet these are the same thing. If you want more waste, each entry has one or two timestamps in it that look like they probably go to the millisecond, however only at second precision, thus giving an extra 3 zeros on the end of each one. There is a lot that could be done to compact this format without resorting to generic compression.

Let's look at a specific example:

----------------------------------------
"[e-mail]":{"d":"/home/dave/.mozilla/firefox/Primary/extensions/[e-mail]","e":true,"v":"4.0.2","st":1414615021000,"mt":1414614987000},
----------------------------------------

one entry, 167B

If we define some simple encoding for default locations, say:

p => profile extension dir
a => application extension dir
/* => any entry starting with a '/' char is an absolute path

store the timestamps in seconds while we're at it, then this can be:

----------------------------------------
"[e-mail]":{"d":"p","e":true,"v":"4.0.2","st":1414615021,"mt":1414614987},
----------------------------------------

one entry, now 91B

Note that on some OSes the path can suck up notably more space and the savings can be larger. (my profile folder name is also shorter than the default)

If you'd like to go a little further, you can use an optimization I've used before for booleans in JSON. For "true" values, store an integer '1' instead. This is truthy, yet only 1 character instead of 4. For "false" values, store nothing instead. The "e" value in this case would then be "undefined", which is also falsy. This saves 3 char for all enabled addons and 10 char for all disabled addons. (including the comma)

With this optimization, this entry can be down to 88B. This is a 47% reduction from the start. Applying this to my profile's pref gets me down to about 3.5KB. This is a total of a 54% reduction in size for the whole pref. (the extra 7% comes from dropping the false booleans for disabled addons)

Conclusion: The current format is wasteful and easily optimized without resorting to a separate file and generic compression.

Comment 4

[Dave Garrett]

If the standard paths aren't available at parse time, they can of course just be put into the JSON file at top-level once each with only a small cost.

Comment 5

[Dave Garrett]

Oh, and if it needs to know which ones are folders vs. files, that's easily dealt with by doubling the path codes. (e.g. "pd" for dir & "px" for XPI, or something)

Comment 6

[Dave Townsend [:mossop]]

I don't think it needs to be as complex as you say, I think we only need to store the paths for the registry locations and when pointer files are in use in the other locations. That would remove the paths for the majority of cases.

Comment 7

[Andrew McCreight [:mccr8]]

FYI we don't send large prefs to the child process (bug 1272707). Hopefully that does not affect the usage of this pref.

Comment 8

[CosminB]

With a large extensions.xpiState preference at startup I received other warning messages in the browser console as presented in the video http://screencast.com/t/Azud6dq34, only when I enable/disable the add-ons I can see the warning related to extensions.xpiState preference.

This issue is reproducing on Firefox 53.0a1 (20170104030214), Firefox 52.0a2 (20170104004006) under Win 7 64-bit and Mac OS X 10.12.1. Release and Beta are not affected.

What do you think about this Dave?

Comment 9

[Dave Townsend [:mossop]]

(In reply to CosminB from comment #8)
> With a large extensions.xpiState preference at startup I received other
> warning messages in the browser console as presented in the video
> http://screencast.com/t/Azud6dq34, only when I enable/disable the add-ons I
> can see the warning related to extensions.xpiState preference.
> 
> This issue is reproducing on Firefox 53.0a1 (20170104030214), Firefox 52.0a2
> (20170104004006) under Win 7 64-bit and Mac OS X 10.12.1. Release and Beta
> are not affected.
> 
> What do you think about this Dave?

I don't think those errors are related to this issue.

Comment 10

[CosminB]

Since I was unable to reproduce this issue on Firefox 53.0a1 (20170104030214), Firefox 52.0a2 (20170104004006) under Win 7 64-bit and Mac OS X 10.12.1 (release and beta seems to not be affected by this issue) I will mark it as WFM. 

If anyone can reproduce this issue on latest versions, feel free to reopen it and provide more information.

Comment 11

[Dave Townsend [:mossop]]

This is still an issue but I don't think it is worth fixing.

Comment 12

[Dave Townsend [:mossop]]

Gijs convinced me that this is worth looking at more since it is a synchronous performance hit everytime we save prefs which is basically every shutdown.

Comment 13

[Dave Townsend [:mossop]]

I've written a patch that uses a json file on disk instead of storing this data in prefs. The numbers are not high confidence but it looks like this causes a between 0.5 and 1% ts_paint performance regression. 
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ee905a096799&newProject=try&newRevision=6b08c990d7b8&framework=1&showOnlyImportant=0

This isn't that surprising to me, although we're loading less data from prefs I wouldn't expect that to make up for the extra file stats and I/O we have to do to load an entirely new file from disk.

What we don't have is any shutdown performance measurements where we think this patch might help because preferences are smaller and so we write less data to disk on shutdown. My guess though is that unless this data gets very large (lots of add-ons installed) not including it isn't going to make much of a difference to the time to write prefs to disk.

Unless we can get some numbers to show what shutdown impact this has I'm inclined to not proceed here but I'd welcome other viewpoints.

Ehsan, do you have any thoughts on this?

Comment 14

[(no longer active)]

(In reply to Dave Townsend [:mossop] from comment #13)
> I've written a patch that uses a json file on disk instead of storing this
> data in prefs. The numbers are not high confidence but it looks like this
> causes a between 0.5 and 1% ts_paint performance regression. 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=ee905a096799&newProject=try&newR
> evision=6b08c990d7b8&framework=1&showOnlyImportant=0
> 
> This isn't that surprising to me

Ugh.  But yeah, sadly it's not surprising.

> although we're loading less data from
> prefs I wouldn't expect that to make up for the extra file stats and I/O we
> have to do to load an entirely new file from disk.

The timing of the I/O can also change how quickly the OS can do it for us, but you're right here, especially if you're on a spinning disk, which many of our users would be.

> What we don't have is any shutdown performance measurements where we think
> this patch might help because preferences are smaller and so we write less
> data to disk on shutdown. My guess though is that unless this data gets very
> large (lots of add-ons installed) not including it isn't going to make much
> of a difference to the time to write prefs to disk.
> 
> Unless we can get some numbers to show what shutdown impact this has I'm
> inclined to not proceed here but I'd welcome other viewpoints.
> 
> Ehsan, do you have any thoughts on this?

I think we should definitely fix this, but in a way that doesn't regress startup performance.  :-)

Looking at the code, this is really out of my area of knowledge, but I think this is what reads the file? <https://hg.mozilla.org/try/rev/6fc2896112a8be6b3415a7d1eff09edf9a4e1214#l2.178>  I don't understand what this code is doing under the hood and why it's so slow, but presumably it's due to synchronous I/O.  Can we use asynchronous I/O instead?

Comment 15

[(no longer active)]

(Also, any chance you can capture this under a profile so that we can see what's going on under the hood here to avoid having to guess?  I at least don't know much about the internals of these APIs.)

Comment 16

[Dave Townsend [:mossop]]

(In reply to :Ehsan Akhgari from comment #14)
> Looking at the code, this is really out of my area of knowledge, but I think
> this is what reads the file?
> <https://hg.mozilla.org/try/rev/6fc2896112a8be6b3415a7d1eff09edf9a4e1214#l2.
> 178>  I don't understand what this code is doing under the hood and why it's
> so slow, but presumably it's due to synchronous I/O.  Can we use
> asynchronous I/O instead?

The root problem is that we need this data synchronously during startup so we can detect new extensions and write out extensions.ini before the chrome registry needs it. There isn't any way to make this asynchronous.

Comment 17

[(no longer active)]

(In reply to Dave Townsend [:mossop] from comment #16)
> (In reply to :Ehsan Akhgari from comment #14)
> > Looking at the code, this is really out of my area of knowledge, but I think
> > this is what reads the file?
> > <https://hg.mozilla.org/try/rev/6fc2896112a8be6b3415a7d1eff09edf9a4e1214#l2.
> > 178>  I don't understand what this code is doing under the hood and why it's
> > so slow, but presumably it's due to synchronous I/O.  Can we use
> > asynchronous I/O instead?
> 
> The root problem is that we need this data synchronously during startup so
> we can detect new extensions and write out extensions.ini before the chrome
> registry needs it. There isn't any way to make this asynchronous.

That's unfortunate. :(

According to comment 3, this data should compress pretty well.  Have you tried storing it in a compressed format to reduce the I/O load?

Comment 18

[Dave Townsend [:mossop]]

Attachment: Bug 1117858: Use JSONFile to persist the XPIStates database.

Review commit: https://reviewboard.mozilla.org/r/118794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118794/

Comment 19

[Dave Townsend [:mossop]]

We unfortunately don't have any already existing method for synchronous decompression of data that would allow us to compress the data here.

Given that we're likely to be able to rearchitect our way out of this after XUL add-ons go away we think it isn't worth working on this for now. I've put up the patch I was testing with in case anyone wanted to pick it back up in the future.