Closed Bug 529074 Opened 10 years ago Closed 10 years ago

Remove use of <preferences> and <preference> from <setting>

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file, 1 obsolete file)

We carry a lot of overhead by using a <preferences> and <preference> for each <setting> element. We should remove both elements and add some preference support code into the <setting> element directly.

The current overhead is causing a speed problem when showing/initializing <setting> elements
Attached patch patch (obsolete) — Splinter Review
This patch removes all usage of <preferences> and <preference>. It adds direct usage of the preference API to the binding. I did some timing on the patch by testing the affect on Ts with the pref panel not hidden so it would initialize during startup:

without patch: 8099ms avg
with patch:    7483ms avg

We save 616ms of preference initialization with the patch. I would expect this savings to also be present with our current configuration, where the pref panel hidden until displayed - which causes initialization to occur on display instead of startup.
Assignee: nobody → mark.finkle
Attachment #412789 - Flags: review?(gavin.sharp)
Comment on attachment 412789 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings/setting.xml b/chrome/content/bindings/setting.xml

>+      <field name="usePref">this.hasAttribute("pref");</field>
>+      <field name="pref">this.getAttribute("pref");</field>
>       <field name="type">this.getAttribute("type");</field>

This is kind of abnormal, because these will only be evaluated once so attribute changes won't be observed... should probably be properties instead.

>+      <field name="service">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefService);
>+      </field>
>+      <field name="branch">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch);
>+      </field>
>+      <field name="branch2">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch2);
>+      </field>

these can all be collapsed to a single member (prefservice can be QIed to prefbranch/prefbranch2)

I think it would help perf-wise to have a global element (referenced from each child using either a field based on ID or .parentNode or something) that has a single reference to the pref service, and handles updates using a single observer - though maybe that introduces to much additional complexity?
(In reply to comment #2)
> (From update of attachment 412789 [details] [diff] [review])
> >diff --git a/chrome/content/bindings/setting.xml b/chrome/content/bindings/setting.xml
> 
> >+      <field name="usePref">this.hasAttribute("pref");</field>
> >+      <field name="pref">this.getAttribute("pref");</field>
> >       <field name="type">this.getAttribute("type");</field>
> 
> This is kind of abnormal, because these will only be evaluated once so
> attribute changes won't be observed... should probably be properties instead.

Changed to properties

> >+      <field name="service">
> >+      <field name="branch">
> >+      <field name="branch2">
> 
> these can all be collapsed to a single member (prefservice can be QIed to
> prefbranch/prefbranch2)

Collapsed to 2 fields, with branch2 QI'ing from branch. We weren't using service.

> I think it would help perf-wise to have a global element (referenced from each
> child using either a field based on ID or .parentNode or something) that has a
> single reference to the pref service, and handles updates using a single
> observer - though maybe that introduces to much additional complexity?

I added the single pref service member in the settings binding. The setting binding will now look for a settings parentNode and use the pref service from there, if possible.

Updated are still going through the setting binding because I didn't want to get into branch complexities.
Attached patch patch 2Splinter Review
Updated patch
Attachment #412789 - Attachment is obsolete: true
Attachment #413037 - Flags: review?(gavin.sharp)
Attachment #412789 - Flags: review?(gavin.sharp)
Comment on attachment 413037 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/bindings/setting.xml b/chrome/content/bindings/setting.xml

>   <binding id="settings">

>+      <field name="branch2">

would prefer "prefb" or "prefs"

>   <binding id="setting-base">

>+      <destructor><![CDATA[
>+        if (this.usePref)
>+          this.branch2.removeObserver(this.pref, this);
>+      ]]></destructor>

There are some cases where XBL destructors won't be called (I always forget exactly which). Use a weak ref instead (and add nsISupportsWeakReference to implements="")?

>+      <method name="inputChanged">

>+            this.fireEvent("onsynctopreference");

>+      <method name="preferenceChanged">

>+            this.fireEvent("onsyncfrompreference");

I guess this is an existing problem, but the toolkit versions of these events let you actually modify the value they'll use (on either the pref or the input). That may not be necessary for us, but since we don't do that, perhaps we should use different event names to avoid confusion? "inputchanged"/"preferencechanged" ?

>+      <field name="branch">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch);
>+      </field>
>+      <field name="branch2">
>+        this.settings ? this.settings.branch2 : this.branch.QueryInterface(Components.interfaces.nsIPrefBranch2);
>+      </field>

Still don't like having these separate. You can just use nsIPrefBranch2 for everything, I think (and call it prefb?).

>   <binding id="setting-bool" extends="chrome://browser/content/bindings/setting.xml#setting-base">
>+      <field name="inverted">this.getAttribute("inverted");</field>

>   <binding id="setting-boolint" extends="chrome://browser/content/bindings/setting.xml#setting-base">
>+      <field name="inverted">this.getAttribute("inverted");</field>

use a property?

>   <binding id="setting-string" extends="chrome://browser/content/bindings/setting.xml#setting-base">

>+          this.value = this.branch
>+                           .getComplexValue(this.pref, Components.interfaces.nsISupportsString)

>+          this.preferences.branch
>+              .setComplexValue(this.pref, Components.interfaces.nsISupportsString, iss);

use Ci? Or maybe we don't want to depend on that, I guess...

r=me with those addressed.
Attachment #413037 - Flags: review?(gavin.sharp) → review+
(In reply to comment #5)
> >+      <field name="branch2">
> 
> would prefer "prefb" or "prefs"

prefs

> >+      <destructor><![CDATA[
> >+        if (this.usePref)
> >+          this.branch2.removeObserver(this.pref, this);
> >+      ]]></destructor>
> 
> There are some cases where XBL destructors won't be called (I always forget
> exactly which). Use a weak ref instead (and add nsISupportsWeakReference to
> implements="")?

Added a <field> object that implemented the weak reference listener. I saw other XBL files do it this way. Seems to work. Removed the destructor.

> >+            this.fireEvent("onsynctopreference");
> >+            this.fireEvent("onsyncfrompreference");
> 
> I guess this is an existing problem, but the toolkit versions of these events
> let you actually modify the value they'll use (on either the pref or the
> input). That may not be necessary for us, but since we don't do that, perhaps
> we should use different event names to avoid confusion?
> "inputchanged"/"preferencechanged" ?

Renamed

> >+      <field name="branch">
> >+      <field name="branch2">
> 
> Still don't like having these separate. You can just use nsIPrefBranch2 for
> everything, I think (and call it prefb?).

Switched to nsIPrefBranch2 only and named the field "_prefs"

> >+      <field name="inverted">this.getAttribute("inverted");</field>
> >+      <field name="inverted">this.getAttribute("inverted");</field>
> 
> use a property?

Done.

> use Ci? Or maybe we don't want to depend on that, I guess...

We don't depend on Ci or Cc in any other XBL and m-c doesn't seem to either. Hopefully taras' work will make those calls faster.
Mardak - this patch changes the settings API a little. It renames the syncto and sycnfrom events. Just a heads up for Weave testing.
pushed:
https://hg.mozilla.org/mobile-browser/rev/bc1395dbbbcb

Needs some browser-tests
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
>+    <implementation implements="nsIObserver">

need to get rid of this now that the object itself doesn't implement nsIObserver

>+      <field name="_prefs">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch2);
>+      </field>

lost the optimization of looking for this.settings.prefs?
The options are faster than the previous day's nightly build. This might not be a result of this bug, but the restart notification within the add-ons manager takes a longer time to pop-down (both after clicking on an uninstall button or installing the add-on as well as during the pop-down showing up). If this is something that someone else can see on their n900, we may need to file a follow-up bug on it.

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b4pre) Gecko/20091123 Firefox/3.6b4pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091123 Firefox/3.7a1pre Fennec/1.0b5
Status: RESOLVED → VERIFIED
(In reply to comment #11)
> The options are faster than the previous day's nightly build

Great!


> the restart notification within the add-ons manager
> takes a longer time to pop-down (both after clicking on an uninstall button or
> installing the add-on as well as during the pop-down showing up).

Vivien has a patch to the notification system (in toolkit) that will make this a bit faster. Right now, the notification tries to "animate" into view, like on desktop firefox. We don't need such things on mobile, especially since the animation only slows us down.
I see, I couldn't find the bug Id for it when searching for "notification" in bugzilla. Can you post the bug id here? Thanks.
(In reply to comment #13)
> I see, I couldn't find the bug Id for it when searching for "notification" in
> bugzilla. Can you post the bug id here? Thanks.

Bug 509732
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.