each nsDocShell should not register as a pref observer

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dbaron, Assigned: nzr)

Tracking

({perf})

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

Every nsDocShell object (one per frame) registers itself as a preference observer, and then unregisters on destruction.  This unregistration uses a search through a linked list (something that we should perhaps also fix).  This can add up to a noticeable amount of time when a lot of tabs are open; see the end of bug 906269 comment 0.

This observer does not manipulate any state that needs to be per-docshell; the current value of the single preference being observed could easily be in a global variable, although it would require slight refactoring of the mUseErrorPages (to reflect three states of use-preference, false, and true, possibly doing that with two booleans).

This should be converted into a single preference observer rather than having one observer for every nsDocShell (which is also bad for performance in the case of the preference actually being changed).
No longer depends on: 906281
Yes to this bug, but we should also just fix the pref observers so they aren't horribly slow.
I'd think a pref var cache would do the job here (created when the first docshell is created).
Whiteboard: [mentor=bz][lang=c++]
blocking-b2g: --- → leo?
triage: not blocking leo, please renominate with justification if you feel this should block release.
blocking-b2g: leo? → -
Hi, i'm interested in doing this for my first bug, if it's okay :)
I've read the getting started guide, and have built mozilla-central (browser).

Currently I'm reading about how docshell works and the properties around it, and I would love to know where to start on this. Thanks!
nsDocShell::Create calls Preferences::AddStrongObserver: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4917
Instead, it would be better to use the preference cache API: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/Preferences.h#265
Hi, Josh. Thanks for replying! :D
Sorry for late reply; I've had quite a struggle with my ISP.

So, what I understand at the moment is I need to add mUseErrorPages to pref var cache *only* when the first docshell is created. For this to work (for all docshells, the variable mUseErrorPages should be changed to a static variable. Then, I need to clean up nsDocShell.cpp (and .h) for usages of Preferences::AddStrongObserver and Preferences::RemoveObserver (and possibly nsDocShell::Observe ?)

In summary, this is what I have to do currently:
- Make the mUseErrorPages attribute static.
- Replace Preferences::AddStrongObserver with Preferences::AddBoolVarCache in nsDocShell::Create,
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4943
- Remove Preferences::RemoveObserver in nsDocShell::SetUseErrorPages and nsDocShell::Destroy,
  http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2585 and #4975

I'm still reading here and there, though. Hopefully I'm in the right direction!
That doesn't sound quite right - mUseErrorPages should remain a non-static member, because SetUseErrorPages can be called on some docshells, causing them to fall out of sync (intentionally) with the pref value. Just add another static member for the cached pref value instead of changing mUseErrorPages.
@Josh

Noted. Alright, I'll start working on it.
And one more thing, What should I do with nsDocShell::Observe?
Since the observer methods will be removed, I don't see the use of implementing observer on Docshell.
Yep. Kill the inheritance.
Posted patch patch_906282.patch (obsolete) — Splinter Review
Attachment #813769 - Flags: review?(josh)
Posted patch p906282_nzr.patch (obsolete) — Splinter Review
What's done (in summary):
1. Replaced pref observer with var cache.
2. Removed nsIObserver inheritance from docshell.
Attachment #813827 - Flags: review?(josh)
Attachment #813827 - Flags: review?(bzbarsky)
Attachment #813827 - Attachment is obsolete: true
Attachment #813827 - Flags: review?(josh)
Attachment #813827 - Flags: review?(bzbarsky)
Posted patch p906282_nzr(v2).patch (obsolete) — Splinter Review
What's done (in summary):
1. Replaced pref observer with var cache.
2. Removed nsIObserver inheritance from docshell.
3. (v2) added aDefault parameter for AddBoolVarCache usage.
Attachment #813832 - Flags: review?(josh)
Attachment #813832 - Flags: review?(bzbarsky)
Attachment #813832 - Attachment description: p906282_nzr (v2).patch → p906282_nzr(v2).patch
Comment on attachment 813832 [details] [diff] [review]
p906282_nzr(v2).patch

>+    bool tUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;
>+    *aUseErrorPages = tUseErrorPages;

How about just:

  *aUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;

?

>+    bool tUseErrorPages = mObserveErrorPages ? sUseErrorPages : mUseErrorPages;

Or better yet, add an inline method on nsDocShell called UseErrorPages() that will return the right boolean, instead of duplicating the ?: bit?

>+    // This boolean replaces docshell's pref observer, as filed in bug
> #908262.

The blame will have the bug number.  How about:

  // Cached value of the "browser.xul.error_pages.enabled" preference.

for the comment here?

r=me with those nits.
Attachment #813832 - Flags: review?(bzbarsky) → review+
Hi, bz. Thanks for reviewing!
I revised the patch by following your review notes:
1. Modified comments.
2. Added inline method UseErrorPages().
Attachment #813962 - Flags: review?(bzbarsky)
Comment on attachment 813962 [details] [diff] [review]
p906282_nzr(v2-revised).patch

r=me
Attachment #813962 - Flags: review?(bzbarsky) → review+
Assignee: nobody → david.christiandy
Status: NEW → ASSIGNED
Feels good to solve my first bug!
Many thanks for all the help, guys. Sorry if it took so long for me to fix.
Attachment #813769 - Attachment is obsolete: true
Attachment #813832 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0e2144372236
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 813962 [details] [diff] [review]
p906282_nzr(v2-revised).patch

[Just some style nits, don't bother fixing them unless a module owner or peer thinks that it's necessary.]

>     if (mObserveErrorPages) {
>         mObserveErrorPages = false;
>     }
I wonder whether it's worth removing the conditional here in SetUseErrorPages.

>     mUseErrorPages =
>         Preferences::GetBool("browser.xul.error_pages.enabled", mUseErrorPages);
Technically we never use this value; UseErrorPages doesn't read mUseErrorPages unless you call SetUseErrorPages which writes it, while Preferences::AddBoolVarCache will read the pref for you anyway.

>+    if(!gAddedPreferencesVarCache) {
Mozilla style is to have a space between keywords such as if and the (.

>     // Remove our pref observers
>     if (mObserveErrorPages) {
>-        Preferences::RemoveObserver(this, "browser.xul.error_pages.enabled");
>         mObserveErrorPages = false;
>     }
And in Destroy I wonder whether it's worth resetting mObserveErrorPages.
You need to log in before you can comment on or make changes to this bug.