Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Session Restore
--
minor
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: zpao, Assigned: marco)

Tracking

({perf})

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P3])

Attachments

(1 attachment, 1 obsolete attachment)

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

8 years ago
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).

Comment 3

8 years ago
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]

Updated

6 years ago
Whiteboard: [snappy] → [snappy:p4]
(Assignee)

Comment 5

6 years ago
Created attachment 576233 [details] [diff] [review]
Patch
Attachment #576233 - Flags: review?(paul)
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.
Attachment #576233 - Flags: review?(paul) → review-
(Assignee)

Comment 7

6 years ago
Created attachment 576252 [details] [diff] [review]
Patch v2

Thank you for your fast review :D
Assignee: nobody → mar.castelluccio
Attachment #576233 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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+

Updated

6 years ago
Whiteboard: [snappy:p4] → [Snappy:P3]
https://hg.mozilla.org/integration/fx-team/rev/a5879751101e
Whiteboard: [Snappy:P3] → [Snappy:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a5879751101e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Snappy:P3][fixed-in-fx-team] → [Snappy:P3]
Target Milestone: --- → Firefox 11

Updated

5 years ago
Depends on: 758138
You need to log in before you can comment on or make changes to this bug.