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.

(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 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.

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.

(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.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f344481977 Remove useless main-thread IO from XUL store; r=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().

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.

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?

Backout by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/acc8ba5c16d4 Backout for test failures

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.

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)

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.)

(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

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.

(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.

(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.

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

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

Comment on attachment 8875781 [details] [diff] [review] Remove support for importing localstore.rdf at startup Review of attachment 8875781 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/95ca02ed62c4 Remove support for importing localstore.rdf at startup; r=florian