Closed Bug 1389344 Opened 7 years ago Closed 7 years ago

Avoid checking for distribution.ini existence on every startup

Categories

(Firefox :: Distributions, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [reserve-photon-performance])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1381594 +++

We do a main-thread check for the distribution.ini file on every startup. We could avoid that and only perform this check after there's a version update, because the update could have added or removed this file.

We could also move this out of the main thread, which is doable but a bit more complicated, and this simpler change will avoid the majority of the penalty that this file stat causes.
Flags: qe-verify?
Comment on attachment 8896068 [details]
Bug 1389344 - Avoid checking for distribution.ini existence on every startup.

Ok, so this patch does the trick but it's not really flexible for testing (due to the loadFromProfile part). I'll modify it tomorrow and write a new test for this.

Any drive-by feedback is still welcome
Attachment #8896068 - Flags: review?(mozilla)
Whiteboard: [photon-performance] → [reserve-photon-performance]
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Comment on attachment 8896068 [details]
Bug 1389344 - Avoid checking for distribution.ini existence on every startup.

https://reviewboard.mozilla.org/r/167346/#review186214

::: browser/components/distribution.js:55
(Diff revision 3)
> +
> +  get _hasDistributionIni() {
> +    if (Services.prefs.prefHasUserValue(PREF_CACHED_FILE_EXISTENCE)) {
> +      let knownForVersion = Services.prefs.getStringPref(PREF_CACHED_FILE_APPVERSION, "unknown");
> +      if (knownForVersion == AppConstants.MOZ_APP_VERSION) {
> +        return Services.prefs.getBoolPref(PREF_CACHED_FILE_EXISTENCE);

What happens if the distribution.ini is removed for some reason? Will it still try to parse?

I'm thinking of the case where someone had a Firefox with a distribution and installed a version that did not.
It will call `Cu.reportError("Unable to parse distribution.ini");`, but otherwise will be harmless (this._ini will remain null, the same way that it would if the file doesn't exist). I'll see if the INI parser returns a proper "file not existing" error that I can filter out and not even report it.
(In reply to :Felipe Gomes (needinfo me!) [offline Sep-20 to Oct-1] from comment #8)
> Comment on attachment 8896068 [details]
> Bug 1389344 - Avoid checking for distribution.ini existence on every startup.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/167346/diff/3-4/

Done what was described in comment 7.

The try run from comment 5 is green.
Comment on attachment 8896068 [details]
Bug 1389344 - Avoid checking for distribution.ini existence on every startup.

https://reviewboard.mozilla.org/r/167346/#review188882
Attachment #8896068 - Flags: review?(mozilla) → review+
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/autoland/rev/b9d7cdb3e163
Avoid checking for distribution.ini existence on every startup. r=mkaply
Backed out for eslint failure at browser/components/tests/unit/test_distribution_cachedexistence.js:24: Cu.import imports into variables and into global scope:

https://hg.mozilla.org/integration/autoland/rev/8acec43298ab5a8477cc7bd7d936bb26e7dd0437

Push with failure: https://hg.mozilla.org/integration/autoland/rev/8acec43298ab5a8477cc7bd7d936bb26e7dd0437
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133411555&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/tests/unit/test_distribution_cachedexistence.js:24:34 | Cu.import imports into variables and into global scope. (mozilla/no-import-into-var-and-global)

Needinfo for florian because felipe doesn't accept needinfo requests at the moment.
Flags: needinfo?(florian)
I have the fix for this, but I can't reopen the review request to push:

-  let {DistributionCustomizer} = Cu.import("resource:///modules/distribution.js");
+  let tmp = {};
+  let {DistributionCustomizer} = Cu.import("resource:///modules/distribution.js", tmp);
   let distribution = new DistributionCustomizer();
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4954eb5975f5
Avoid checking for distribution.ini existence on every startup. r=mkaply
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #12)

Too bad I missed that failure when looking at the try results :-(.

> Needinfo for florian because felipe doesn't accept needinfo requests at the
> moment.

Yes, felipe asked me to take care of landing this while he's in vacations.
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/4954eb5975f5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8896068 [details]
Bug 1389344 - Avoid checking for distribution.ini existence on every startup.

Requesting beta approval on felipe's behalf while he's away, I know he was targeting this fix at 57.

Approval Request Comment
[Feature/Bug causing the regression]: not a recent regression.
[User impact if declined]: useless main thread IO at each startup, instead of only after an update.
[Is this code covered by automated tests?]: yes.
[Has the fix been verified in Nightly?]: no.
[Needs manual test from QE? If yes, steps to reproduce]: I think QA doing some exploratory testing to verify that we are not somehow breaking distribution-specific behaviors would be nice.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: I can't say "no" for something that runs in the startup  path, but I think it's still acceptable at this early time of the beta cycle.
[Why is the change risky/not risky?]: There's a slight risk because this is touching startup in a way that nightly testers won't exercise. However, this has good automated test coverage (actually most of the patch is test changes), and the code change is otherwise self contained (just caching the result of the "this._iniFile.exists()" call in a preference).
[String changes made/needed]: none.
Attachment #8896068 - Flags: approval-mozilla-beta?
Comment on attachment 8896068 [details]
Bug 1389344 - Avoid checking for distribution.ini existence on every startup.

Sorry, I am (even) more conservative with this cycle than usual. Especially as we now have data which shows that 50% of perf uplifts will introduce regressions.
So, as this isn't a new regression, we should let it ride the train with 58
Attachment #8896068 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Florian Quèze [:florian] [:flo] from comment #17)
> [Needs manual test from QE? If yes, steps to reproduce]: I think QA doing
> some exploratory testing to verify that we are not somehow breaking
> distribution-specific behaviors would be nice.

I think some specific details around partner builds would do well here, since we lack the context for testing around distributions and not sure where, if or what information is available. 

- do we have partner builds for Nightly/Beta? If yes, where do we get them?
- where would the distribution.ini would be located - my assumption would be install location?
- in 58, I see in about:config there are two configs: distribution.iniFile.exists.appversion - 58.0a1 and distribution.iniFile.exists.value - "false" and the two prefs do not exist in 57.0b5. Setting the "distribution.iniFile.exists.value" to true, will remove the pref entirely.

If my memory serves me right, I think I've nudged :mkaply about partner builds specifics while testing OutOfDate Notification System Add-on a while back, so I'm going to do the same for the above as well. Mike, can you please check the questions above?
Flags: needinfo?(mozilla)
Unfortunately we do not have partner repacks for nightly, so this can't be tested easily on that front.

I do have beta repacks, but this didn't get checked in there.

I can certainly do some one offs (unsigned on Windows) so that you can do some testing if you would like.

Do we need to do this right now, though, or should we wait until this is on beta 58?
Flags: needinfo?(mozilla)
Mike, since we are now on beta 58, I think we can pick up on verifying this? Could you link me to any beta partners repacks?
Flags: needinfo?(mozilla)
Sure:

Any of the builds here should work:

http://archive.mozilla.org/pub/firefox/candidates/58.0b4-candidates/build1/partner-repacks/
Flags: needinfo?(mozilla)
I tried to see if there were any difference in performance between one of the builds from comment 22 and older ironsource builds (in this case 56.0b11 and 56.0b7) on Windows 10 x64. I did a bit of exploratory, especially on Bookmarks and Customize, but I couldn't find any visible differences. 
I also tried to change entries in distribution.ini document, but the changes remained the same even if I updated the browser. To be more clear on this matter, if I changed a word in the document that word remained changed even after update. 
 
Will this result suffice to close the bug as verified? Or could you recommend another method to verify it?
Thank you.
Flags: needinfo?(felipc)
On Linux I would use strace (and grep) to verify that we were doing syscalls related to distribution.ini before the patch, and that we are no longer doing them with the patch. I assume equivalents of strace exist on other platforms but I don't know their names.
I don't think this bug needs QA verification. The test that landed with it reproduces the scenarios where the file was deleted to fool the code and make sure it's not re-reading the file. It also reproduces then the case of a version update to make sure that the file is checked again after an update.
Flags: needinfo?(felipc)
Removing qe+ as per comment 25.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: