Closed Bug 1375870 Opened 7 years ago Closed 7 years ago

_constructAfterChildren in preferences.xml should be called only once

Categories

(Firefox :: Settings UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 56
Iteration:
56.4 - Aug 1
Tracking Status
firefox56 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Depends on 1 open bug)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 file)

Presently this function is called after the construction of every <preference> element. Each time, it checks whether all such <preference> elements have been constructed, and only executes its real payload if that is the case. That is, it really just wants to be called once after the last <preference> has been constructed. This caused >80ms of overhead in my local build on a fast Macbook Pro: https://perf-html.io/public/fa785622d686a49c585ce7a8b8d3c7fdf92f1812/calltree/?callTreeFilters=prefixjs-yB5yBlyBoyBpyBqyC1&implementation=js&thread=0

I propose to use a count to detect the construction of the final <preference> element and call _constructAfterChildren only once.
Whiteboard: [reserve-photon-performance]
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: qe-verify-
Priority: -- → P1
Comment on attachment 8880845 [details]
Bug 1375870 - _constructAfterChildren in preferences.xml should be called only once.

Redirecting review since I don't have time to look into this
Attachment #8880845 - Flags: review?(MattN+bmo) → review?(dtownsend)
Not entirely against this approach but I have a couple of thoughts and wondered if you considered them:

Do we actually need to defer this work until after all the elements are constructed? I can't see an obvious reason why that is. If there is a reason we should probably document it in comments somewhere.

Did you happen to try making the getElementsByTagName a field on <preferences>? As a live node list it will be updated automatically as new preference elements are added and it might be that iterating the list is cheap while constructing it is expensive.

The current code handles the case where <preference> elements are dynamically added to the DOM while your new code doesn't currently. Is that something we do anywhere?
(In reply to Dave Townsend [:mossop] from comment #3)
> The current code handles the case where <preference> elements are
> dynamically added to the DOM while your new code doesn't currently. Is that
> something we do anywhere?

I know some system add-ons (autofill and screenshots) would like to but IIRC for autofill we had an issue that made the pref checkbox not work properly when dynamically added. Regardless, I think with the move of some things to system add-ons it won't be unheard of to want to dynamically add to preference panes.
Comment on attachment 8880845 [details]
Bug 1375870 - _constructAfterChildren in preferences.xml should be called only once.

https://reviewboard.mozilla.org/r/152188/#review160840

Waiting on answers to questions here
Attachment #8880845 - Flags: review?(dtownsend)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > The current code handles the case where <preference> elements are
> > dynamically added to the DOM while your new code doesn't currently. Is that
> > something we do anywhere?
> 
> I know some system add-ons (autofill and screenshots) would like to but IIRC
> for autofill we had an issue that made the pref checkbox not work properly
> when dynamically added. Regardless, I think with the move of some things to
> system add-ons it won't be unheard of to want to dynamically add to
> preference panes.

The about:preferences page currently does this in two gMainPane methods, _readDefaultFontTypeForLanguage and _selectDefaultLanguageGroup:

http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/components/preferences/in-content-new/main.js#719-725
http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/components/preferences/in-content-new/main.js#752-757
I wonder if we could have this or bug 1379338 reaching Fx56, since with the new Preferences the # of nodes gets longer in each pane.

Also, can we workaround this by using multiple <preferences> in each pane? We'll at least be able to get things about the fold to show up first.
(In reply to Dave Townsend [:mossop] from comment #3)
> Not entirely against this approach but I have a couple of thoughts and
> wondered if you considered them:
> 
> Do we actually need to defer this work until after all the elements are
> constructed? I can't see an obvious reason why that is. If there is a reason
> we should probably document it in comments somewhere.

After looking at this code for a while, I don't think we do. As long as the form elements are available in the DOM at the time that updateElements is called, we could just call updateElements directly in the constructor of <preference>. I tried this change and it didn't seem to break anything. My next step is going to be to assert the hypothesis that calling updateElements directly in the constructor doesn't leave out any form elements that might be added later.

> Did you happen to try making the getElementsByTagName a field on
> <preferences>? As a live node list it will be updated automatically as new
> preference elements are added and it might be that iterating the list is
> cheap while constructing it is expensive.

Good point, thanks, but we may not need to do anything of the sort if we can just call updateElements directly in <preference>

> The current code handles the case where <preference> elements are
> dynamically added to the DOM while your new code doesn't currently. Is that
> something we do anywhere?

Calling updateElements in the <preference> constructor will eliminate this problem.
Manish, seems like you wrote the current _constructAfterChildren code. Do you by any chance remember why the code is waiting for all child <preference> elements have been constructed before calling updateElements on each one?
Flags: needinfo?(manishearth)
We have to defer because sometimes the code for some pref initializers depend on other preference elements being initialized. See bug 997570 (and bug 992185 for an example)
Flags: needinfo?(manishearth)
More accurately, some of the handlers that get transitively called when a <preference> is initialized depend on other preference elements being initialized.
(In reply to Dave Townsend [:mossop] from comment #3)
> Not entirely against this approach but I have a couple of thoughts and
> wondered if you considered them:
> 
> Do we actually need to defer this work until after all the elements are
> constructed? I can't see an obvious reason why that is. If there is a reason
> we should probably document it in comments somewhere.

See Manish's comment 11. Hmm, forgot to write a comment, will do in next patch.

> Did you happen to try making the getElementsByTagName a field on
> <preferences>? As a live node list it will be updated automatically as new
> preference elements are added and it might be that iterating the list is
> cheap while constructing it is expensive.

Good idea, thanks. This also makes sure that _constructAfterChildren is called if a <preference> is added dynamically later, which addresses your next question:

> The current code handles the case where <preference> elements are
> dynamically added to the DOM while your new code doesn't currently. Is that
> something we do anywhere?

Seems like we do, but maintaining a live node list takes care of it.

Thinking about it, a further optimization is necessary for this case:

> 20 <preference> elements exist
> 20 <preference> elements are constructed
> _constructAfterChildren is called, it calls updateElements on all child <preference> elements
> A new <preference> element is dynamically added.
> The count matches the node list length so _constructAfterChildren is called again, causing updateElements to be called on ALL child <preference> elements again. This is bad.

So I think we need a field in the <preferences> binding that indicates if _constructAfterChildren has already been called, and if it is true, the <constructor> of <preference> should directly call updateElements to avoid looping through all other <preference> children.
Comment on attachment 8880845 [details]
Bug 1375870 - _constructAfterChildren in preferences.xml should be called only once.

https://reviewboard.mozilla.org/r/152188/#review166832

::: toolkit/content/widgets/preferences.xml:180
(Diff revision 3)
> +        }
> +        this.preferences._constructedChildrenCount++;
> +        if (this.preferences._constructedChildrenCount ==
> +            this.preferences._preferenceChildren.length) {
> +          // This is the last <preference>, time to updateElements().
> +          if (!this.preferences._constructAfterChildrenCalled) {

You should do this test first, it's cheap and if _constructAfterChildrenCalled is true then you don't need to worry about the count.
Attachment #8880845 - Flags: review?(dtownsend)
Comment on attachment 8880845 [details]
Bug 1375870 - _constructAfterChildren in preferences.xml should be called only once.

https://reviewboard.mozilla.org/r/152188/#review168486
Attachment #8880845 - Flags: review?(dtownsend) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d87add06773
_constructAfterChildren in preferences.xml should be called only once. r=mossop
https://hg.mozilla.org/mozilla-central/rev/6d87add06773
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.4 - Aug 1
Depends on: 1423564
Depends on: 1425699
You need to log in before you can comment on or make changes to this bug.