Last Comment Bug 542032 - Don't look up prefs on every tab & window close
: Don't look up prefs on every tab & window close
Status: RESOLVED FIXED
[Snappy:P3]
: perf
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Firefox 11
Assigned To: Marco Castelluccio [:marco]
:
Mentors:
Depends on: 758138
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-25 11:18 PST by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2013-12-27 14:23 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.61 KB, patch)
2011-11-22 12:50 PST, Marco Castelluccio [:marco]
paul: review-
Details | Diff | Review
Patch v2 (6.73 KB, patch)
2011-11-22 14:01 PST, Marco Castelluccio [:marco]
paul: review+
Details | Diff | Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-01-25 11:18:43 PST
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 2010-01-25 14:00:19 PST
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 reading much bugmail, email directly) 2010-01-25 15:21:41 PST
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 2010-01-26 08:41:50 PST
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 reading much bugmail, email directly) 2010-01-26 12:14:48 PST
(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] 2011-11-22 12:50:00 PST
Created attachment 576233 [details] [diff] [review]
Patch
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-22 13:05:48 PST
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] 2011-11-22 14:01:20 PST
Created attachment 576252 [details] [diff] [review]
Patch v2

Thank you for your fast review :D
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-28 13:33:53 PST
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 reading much bugmail, email directly) 2011-12-02 10:22:32 PST
https://hg.mozilla.org/integration/fx-team/rev/a5879751101e
Comment 10 Tim Taubert [:ttaubert] 2011-12-06 00:06:21 PST
https://hg.mozilla.org/mozilla-central/rev/a5879751101e

Note You need to log in before you can comment on or make changes to this bug.