542032 - Don't look up prefs on every tab & window close

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

browser.sessionstore.max_tabs_undo is looked up every time a tab is closed. max_windows_undo is looked up every time a window is closed.

There's no need for that, so we should cache & observe changes.

Comment 1

[Simon Bünzli]

This sounds as if occasionally looking up a pref were a bad thing. Do you have data showing that these lookups are in any way less efficient than installing yet another observer?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

We're already doing 1+ lookups, so we can say those cancel out. So as long as the cost of the observer is less than the cost of all other lookups then this is a win. Creating the observer is likely cheap (maybe on equal costs as 1 or 2 lookups... though that's a completely unfounded statement).

I would say it's definitely worth it for max_tabs_undo. Maybe less so for max_windows_undo. Either way these prefs are unlikely to change more than once per session (probably closer to once in 50 sessions if that).

Comment 3

[Simon Bünzli]

There's also some cost in keeping an observer around (which is what I'm concerned about) and some slight cost in the additional code.

Looking closer, it turns out that we do already have observers in place, though - so all that's left is caching the values...

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #3)
> Looking closer, it turns out that we do already have observers in place, though
> - so all that's left is caching the values...

Indeed (I completely forgot we were doing that). Ideally we'd do this like the prefs in bug 539660 so that it's lazy. That patch caused odd crashes though, so perhaps not.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 576233 [details] [diff] [review]
Patch

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

First off, thanks for starting on this Marco! You're on the right track here, but you haven't solved the whole problem.

Here are the 3 steps to fixing this:
1. Make the pref value an attribute
2. Update the attribute when the pref value changes (see observe)
3. Use the attribute instead of looking up the pref when a tab/window is closed (see onTabClose and _capClosedWindows)

You've done step 1, but the other 2 steps still need to be done.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Thank you for your fast review :D

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 576252 [details] [diff] [review]
Patch v2

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

Looks good to me! Thanks Marco. I'll push this to try server just to make sure and then I'll get it checked in.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/integration/fx-team/rev/a5879751101e

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a5879751101e