1368567 - XULStore does useless main thread IO

Status: RESOLVED FIXED

(Core Graveyard :: RDF, enhancement)

Description

[Florian Quèze [:florian]]

See this cold startup profile: https://perfht.ml/2r59nmF

We check for the file's existence at http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/xulstore/XULStore.js#68 before actually reading the file.

I think we could pay only once the cost of main thread IO here if we just tried to read the file, and let the code reading the file throw an exception that we can catch and identify when the file doesn't exist.

The import code path also does something similar: http://searchfox.org/mozilla-central/rev/31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/xulstore/XULStore.js#96


Of course ideally we would want all of this to use async IO, but removing the .exists() calls seems like an easy short term win.

Comment 1

[(no longer active)]

(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> The import code path also does something similar:
> http://searchfox.org/mozilla-central/rev/
> 31eec6f59eb303adf3318058cb30bd6a883115a5/toolkit/components/xulstore/
> XULStore.js#96

For this code path the RDF component helpfully eats our exception: <http://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/rdf/base/nsRDFXMLDataSource.cpp#488>, but the good news is that you'll just get an empty object so import() won't have any side effects, so we can just delete the block you linked to! :-)

Comment 2

[(no longer active)]

Attachment: Remove useless main-thread IO from XUL store

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8874065 [details] [diff] [review]
Remove useless main-thread IO from XUL store

Review of attachment 8874065 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking at this! :-)

::: toolkit/components/xulstore/XULStore.js
@@ +150,5 @@
>        stream.init(this._storeFile, MODE_RDONLY, FILE_PERMS, 0);
>        this._data = json.decodeFromStream(stream, stream.available());
>      } catch (e) {
>        this.log("Error reading JSON: " + e);
> +      if (e.name == "NS_ERROR_FILE_NOT_FOUND") {

I assume you have tested this, but doing it with a string comparison seems a little bit fragile to me. Most of the existing code doing similar checks uses:
    if (e.result == Cr.NS_ERROR_FILE_NOT_FOUND)

@@ +157,2 @@
>      } finally {
>        stream.close();

Do you know if this will be a no-op when initializing the stream failed with NS_ERROR_FILE_NOT_FOUND? If close does main thread IO even when we failed to open, then we are not winning much with this patch.

I tried to figure it out by looking at http://searchfox.org/mozilla-central/source/netwerk/base/nsFileStreams.cpp but failed.

Comment 4

[Florian Quèze [:florian]]

For a more complete fix in the future, I think we'll want to start reading the xulstore.json file asynchronously as soon as the user profile has been selected, and fallback to a synchronous method (or wait on a promise) when opening the first browser window.

Comment 5

[(no longer active)]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> ::: toolkit/components/xulstore/XULStore.js
> @@ +150,5 @@
> >        stream.init(this._storeFile, MODE_RDONLY, FILE_PERMS, 0);
> >        this._data = json.decodeFromStream(stream, stream.available());
> >      } catch (e) {
> >        this.log("Error reading JSON: " + e);
> > +      if (e.name == "NS_ERROR_FILE_NOT_FOUND") {
> 
> I assume you have tested this, but doing it with a string comparison seems a
> little bit fragile to me. Most of the existing code doing similar checks
> uses:
>     if (e.result == Cr.NS_ERROR_FILE_NOT_FOUND)

Ah, right!  I'm losing everything I used to know about writing chrome JS.  :-)  I was trying to remember how to do this and the only trick I could remember was checking the .name, which, while it works, is a bit inefficient.

(FWIW we do have a test for this stuff: https://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/toolkit/components/xulstore/tests/xpcshell/test_XULStore.js#89.)

> @@ +157,2 @@
> >      } finally {
> >        stream.close();
> 
> Do you know if this will be a no-op when initializing the stream failed with
> NS_ERROR_FILE_NOT_FOUND? If close does main thread IO even when we failed to
> open, then we are not winning much with this patch.

Yes, here is the proof:

nsFileStreamBase::DoOpen() only sets mFD upon success: <https://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/netwerk/base/nsFileStreams.cpp#351>.  This function is indirectly called through nsFileInputStream::Init().  nsFileInputStream::Close() calls nsFileStreamBase::Close() which only calls PR_Close() on mFD (which is what would fail due to the file opening having failed previously) when mFD is set <https://searchfox.org/mozilla-central/rev/3efd1caff252061946f0773df68bdd50de5eb756/netwerk/base/nsFileStreams.cpp#169>.  Hence this is a no-op upon failure.

But I realized that if I had added a test for importing without an existing localstore.rdf, you wouldn't have needed to ask me anything.  :-)  So I'll just do that when landing.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f344481977
Remove useless main-thread IO from XUL store; r=florian

Comment 7

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #5)

> But I realized that if I had added a test for importing without an existing
> localstore.rdf, you wouldn't have needed to ask me anything.  :-)  So I'll
> just do that when landing.

Thanks for adding a test... but I don't think it counts how many times we do main thread IO, so I would probably still have asked the question about .close().

Comment 8

[(no longer active)]

Bah, this broke all sorts of tests :-( https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c7f34448197790499d4709a6e5a36aa7aa876b1b

It seems like even though that function could deal with an empty RDF data store, the rest of our code can't.

Comment 9

[(no longer active)]

Fixing https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f344481977#l1.37 requires changing the code I linked to in comment 1.  That code was added in bug 100132 (in 2001) and isn't something I'm comfortable changing to be honest.

Are you OK with me reverting that hunk and relanding?

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acc8ba5c16d4
Backout for test failures

Comment 11

[Marco Bonardo [:mak]]

Honestly, the migration from localstore to xulstore happened in Firefox 34, we are at 55 (ESR52), that's enough versions to migrate users we care about. Imho the whole import() method should go away.

Comment 12

[Marco Bonardo [:mak]]

note, if we do that, there are a few references that can be removed:
http://searchfox.org/mozilla-central/search?q=localstore.rdf&path=
(everything but rdf and xre... Also testing/talos/talos/base_profile ?!?!?! are we really putting that deprecated file into Talos measurements)

Comment 13

[(no longer active)]

I'm fine with removing the import code.  I guess this falls under toolkit, so Mossop is the decision owner.  (I'd also like to know what Florian thinks, of course.)

Comment 14

[Dave Townsend [:mossop]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #13)
> I'm fine with removing the import code.  I guess this falls under toolkit,
> so Mossop is the decision owner.  (I'd also like to know what Florian
> thinks, of course.)

I'm fine with removing the import code too

Comment 15

[Florian Quèze [:florian]]

So the possible paths forward here are:

1. Land only the first part of the patch. This should help for the general case where we have a valid xulstore.json file.

2. Remove the import code. I think this is a good thing to do, but I'm not sure this bug is the right place for it. Especially as this migration happened in Firefox 34 according to comment 11, and we still have migration code for Firefox 14 and later around http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/browser/components/nsBrowserGlue.js#1729 I would suggest filling a follow-up to review all this migration code and clean it up.

3. Stop calling the import code automatically from XULStore.load, and call it explicitly from the nsBrowserGlue.js _migrateUI method, so that it only runs when we are migrating from the relevant Firefox version.

I'm fine with either of these solutions. Assuming no other work, I have a slight preference for #3, but if we commit to removing old migration code from _migrateUI within the 57 time frame, then #2 is better.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> but I'm not
> sure this bug is the right place for it. Especially as this migration
> happened in Firefox 34 according to comment 11, and we still have migration
> code for Firefox 14 and later around

If we keep delaying removals until someone does all of them, we'll just keep accumulating migrations, and indeed that's already happening. We should try to cleanup the codebase every time we have a chance, in a more aggressive way.

Also note that UI Version 14 is about Firefox 25, not Firefox 14, there's no connection between FF versions and UI migrations since you can do more than one UI migration in a version (or none). We could indeed remove any UI migration up to 20 (included) if we'd want to align on FF 34, but there's no strict reason to align every component on a specific version.
I'm also not sure I see the point made about nsBrowserGlue here. The suggested path is to remove an import method and its call from xulStore.js, that would also simplify the patch and provide a perf win. What's up with a browserGlue discussion?

> 3. Stop calling the import code automatically from XULStore.load, and call
> it explicitly from the nsBrowserGlue.js _migrateUI method, so that it only
> runs when we are migrating from the relevant Firefox version.

What's the point 18 versions (considering ESR) and 2 years after the fact? What are we trying to achieve? for users who didn't open the profile for 2 years localstore is more likely to create issues than benefits.

Comment 17

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #16)

Fair enough, let's remove the import code, and file a follow-up to cleanup the UI migrations.

Comment 18

[(no longer active)]

Filed bug 1371320 as the follow-up to review and remove old UI migration code.

Comment 19

[(no longer active)]

Attachment: Remove support for importing localstore.rdf at startup

This removes some useless main-thread IO from the startup path.

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8875781 [details] [diff] [review]
Remove support for importing localstore.rdf at startup

Review of attachment 8875781 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95ca02ed62c4
Remove support for importing localstore.rdf at startup; r=florian

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95ca02ed62c4