Don't run old migration code every time at startup, only when suite.migration.version doesn't exist yet

RESOLVED FIXED in seamonkey2.52

Status

SeaMonkey
Startup & Profiles
--
minor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

({perf})

Trunk
seamonkey2.52

SeaMonkey Tracking Flags

(seamonkey2.48 unaffected, seamonkey2.49esr fixed, seamonkey2.50 wontfix, seamonkey2.51 fixed, seamonkey2.52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
+++ This bug was initially created as a clone of Bug #1366496 +++

(Quoting Frank-Rainer Grahl from bug 1366496 comment #2)
> I added a _migrateUI to nsSuiteGlue.js in bug 1331477 to avoid running
> migrations every time on startup.

(Quoting rsx11m from bug 1366496 comment #3)
> The other migrations should go there as well, let's do this in a follow-up[...]
(Assignee)

Comment 1

a year ago
Created attachment 8869837 [details] [diff] [review]
Direct fix

So, this is the easiest I can come up with - just moving this._updatePrefs() and this._migrateDownloadPrefs() into _migrateUI() and executing those if there is no user_pref suite.migration.version in the current profile.

I don't see the point in actually moving all the old migrations from the separate functions into _migrateUI(), makes it messier and loses per-line change history.
 
Note that migrateMailnews() is still executed on every startup, that's what Thunderbird does as well, so apparently there are use cases for that.

This patch goes on top of attachment 8869825 [details] [diff] [review] to account for UI_VERSION being bumped in bug 1366496.
Attachment #8869837 - Flags: review?(frgrahl)
Comment on attachment 8869837 [details] [diff] [review]
Direct fix

Yes keeping the old functions is preferred.

You could move the code in the else inside the initial if: 

> if (currentUIVersion < 1) {

Code in it is run the same time as the else because the version is 0 then. Just needs an adjustment of the comment and a new one for the if. Let me know if this is ok for you too.

f+ for now.
Attachment #8869837 - Flags: review?(frgrahl) → feedback+
(Assignee)

Comment 3

a year ago
(In reply to Frank-Rainer Grahl from comment #2)
> You could move the code in the else inside the initial if: 

I was thinking of putting them there first as well, but then decided to make it part of the very first "check if that preference exists" construct. It is more explicit there and thus not hidden in what is essentially the first "real" (currentUIVersion < 1) migration within that function.

Meaning: I'd prefer as is, but it's obviously straight-forward to change the patch as you suggested.
Flags: needinfo?(frgrahl)
While performance is a non issue for both choices my reasoning was/is that all migration code is below:

> if (currentUIVersion >= UI_VERSION)
>       return;

Also one "else" less which reduces complexity by 0.0001% :)

Just let me know your final verdict. We could ask IanN but he would probably call us crazy discussing 2 lines of code. :D
Flags: needinfo?(frgrahl)
(Assignee)

Comment 5

a year ago
Created attachment 8870409 [details] [diff] [review]
Use a single if() only

(In reply to Frank-Rainer Grahl from comment #4)
> > if (currentUIVersion >= UI_VERSION)
> >       return;

Admittedly, this would make it consistent with the other applications, which in turn had the UI_VERSION from the beginning (thus, our "old-style" migration is unique in this sense). Also, the others may have multiple migrations in a single version step, thus one step isn't necessarily just one migration.

> Also one "else" less which reduces complexity by 0.0001% :)

I'd think that a single "branch" instruction over the "else" part is really cheap... ;-)

Anyway, as long as we move the comment inside the "if (currentUIVersion < 1)" clause this still looks reasonably distinguishable. So, let's just go with this and save a few picoseconds.

I've also adjusted capitalization and punctuation in the new comment.
Attachment #8869837 - Attachment is obsolete: true
Attachment #8870409 - Flags: review?(frgrahl)
Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

Looks good. Just as I wanted it :) I think this is very clean looking now.

> So, let's just go with this and save a few picoseconds.

I wouldn't mind to save the whole bunch of picoseconds for the 2 migration routines in 2.49 esr. If you think so too please ask IanN for approval.
Attachment #8870409 - Flags: review?(frgrahl) → review+
(Assignee)

Comment 7

a year ago
Thanks, pushed as https://hg.mozilla.org/comm-central/rev/849228c56d57
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-seamonkey2.48: --- → wontfix
status-seamonkey2.49esr: --- → affected
status-seamonkey2.50: --- → wontfix
status-seamonkey2.51: --- → affected
status-seamonkey2.52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.52
(Assignee)

Comment 8

a year ago
Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

(In reply to Frank-Rainer Grahl from comment #6)
> I wouldn't mind to save the whole bunch of picoseconds for the 2 migration
> routines in 2.49 esr. If you think so too please ask IanN for approval.

Should be simple enough.

[Approval Request Comment]
Regression caused by (bug #): No regression, follow-up to bug 1331477
User impact if declined: startup time is a few picoseconds longer
String changes made by this patch: none
Attachment #8870409 - Flags: approval-comm-esr52?
Attachment #8870409 - Flags: approval-comm-beta?
(Assignee)

Comment 9

a year ago
Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

[Approval Request Comment (cont.)]
Testing completed (on m-c, etc.): SM 2.49 (c-r build just before the last merge)
Risk to taking this patch (and alternatives if risky): very low.

This time I've actually retested the patch on a branch build /before/ claiming that it works there! ;-)
(Assignee)

Updated

a year ago
status-seamonkey2.48: wontfix → unaffected
Keywords: perf

Comment 10

a year ago
Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

[Triage Comment]
a=me for all repos that need it
Attachment #8870409 - Flags: approval-comm-release+
Attachment #8870409 - Flags: approval-comm-esr52?
Attachment #8870409 - Flags: approval-comm-esr52+
Attachment #8870409 - Flags: approval-comm-beta?
Attachment #8870409 - Flags: approval-comm-beta+
(Assignee)

Comment 11

a year ago
Thanks, pushed as
https://hg.mozilla.org/releases/comm-beta/rev/ac2c15271d47
https://hg.mozilla.org/releases/comm-esr52/rev/82ad24f24c2c
status-seamonkey2.49esr: affected → fixed
status-seamonkey2.51: affected → fixed
You need to log in before you can comment on or make changes to this bug.