Closed Bug 542032 Opened 11 years ago Closed 9 years ago

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


(Firefox :: Session Restore, defect)

Not set



Firefox 11


(Reporter: zpao, Assigned: marco)



(Keywords: perf, Whiteboard: [Snappy:P3])


(1 file, 1 obsolete file)

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.
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?
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).
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...
(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.
Severity: normal → minor
Keywords: perf
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [tsnap]
Whiteboard: [tsnap] → [snappy]
Whiteboard: [snappy] → [snappy:p4]
Attached patch Patch (obsolete) — Splinter Review
Attachment #576233 - Flags: review?(paul)
Comment on attachment 576233 [details] [diff] [review]

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.
Attachment #576233 - Flags: review?(paul) → review-
Attached patch Patch v2Splinter Review
Thank you for your fast review :D
Assignee: nobody → mar.castelluccio
Attachment #576233 - Attachment is obsolete: true
Attachment #576252 - Flags: review?(paul)
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.
Attachment #576252 - Flags: review?(paul) → review+
Whiteboard: [snappy:p4] → [Snappy:P3]
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P3][fixed-in-fx-team] → [Snappy:P3]
Target Milestone: --- → Firefox 11
Depends on: 758138
You need to log in before you can comment on or make changes to this bug.