885926 - Switch out querySelector(idToSelector(id)) for getElementsByAttribute(id) for palette queries

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

There are a few times where we query the hidden toolbox palette for an immediate child using querySelector(idToSelector(id)). It turns out that idToSelector is pretty expensive, performance-wise, and we do lots of this querying before we even do first paint (mounting evidence suggests it was a major contributor to the talos regression in bug 880611).

We seem to get far better performance if we just use getElementsByAttribute("id", id) on the palette to query for the children.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

So due to a foul-up on my part, this was erroneously evaluated as an excellent way to recover performance on first paint.

See bug 880611 comment 15 for more details.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike Conley (:mconley) from comment #15)
> I may have broken out the champagne too soon - my patch had a bug where I
> was using element.getElementsByAttribute("id", aId), for a node instead of
> element.getElementsByAttribute("id", aId)[0]. Kinda makes all the
> difference, and now the regression is back. :/

So did that make no difference, or did it have a negative impact? If it made no difference, I think we should consider fixing this bug anyway - getElementsByAttribute is more robust, and will likely benefit from future performance improvements.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I actually do think this would be a cleaner way of doing things, instead of mucking about with regex's. Certainly not a blocker though.

Comment 4

[Sumit Agrawal[:sumit4iit]]

Hi Mike,

I would like to take this bug. Please assign it to me. 

I am new to mozilla, will you please suggest me where to start for solving this bug? 

Thanks

Comment 5

[Sumit Agrawal[:sumit4iit]]

Thanks Jared,

Can you help me with how to start with this bug? I am not sure which files do I need to look into. 

Thanks.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Sure, you'll need to clone the UX repository which is located at https://hg.mozilla.org/projects/ux. The file in question is located at browser\components\customizableui\src\CustomizableUI.jsm. You'll find idToSelector at the bottom of the file, and there are numerous places within the file where the result of idToSelector is passed to querySelector.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Jared's definitely got you on the right track here. Thanks for jumping on this, sumit4iit! Let us know if you need more pointers.

Comment 8

[Sumit Agrawal[:sumit4iit]]

Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I see that most of the code of m-c and UX is similar.
Do I need to download entire UX repository or is there a way to reuse the code from m-c?
I checked wiki for UX branch and did not get much information.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to sumit4iit from comment #8)
> Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I
> see that most of the code of m-c and UX is similar.
> Do I need to download entire UX repository or is there a way to reuse the
> code from m-c?
> I checked wiki for UX branch and did not get much information.

Hey sumit4iit, you're definitely going to want the UX branch. While the vast majority of it is similar to mozilla-central, the part you'll be working on is unique to UX.

You can clone it from here:  http://hg.mozilla.org/projects/ux

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to sumit4iit from comment #8)
> Thanks Jared, I have already cloned "mozilla-central" and compiled it, and I
> see that most of the code of m-c and UX is similar.
> Do I need to download entire UX repository or is there a way to reuse the
> code from m-c?
> I checked wiki for UX branch and did not get much information.

I updated the UX branch wiki page to hopefully make this more clear. Please email me if you think there are still some things that could be explained better.

Comment 11

[Sumit Agrawal[:sumit4iit]]

Thanks Jared, I guess that should help newcomers. I have cloned the repo.
The other issue that I noticed is that (I am not sure that if this is the correct place to report, please let me know), 
While cloning I noticed that many times there was a network reset issue and 'hg' used to rollback the transaction. So I used axel to get source in zip format, and I noticed that mozilla server does not support resume capability. I am not sure if this is actually supposed to happen or if this is a bug. 
And this was the reason why I went for the mozilla-central.

Comment 12

[Emma Sajic]

I'm a software developer new to Mozilla but I have done bug patches for Google Android before. I was just wondering whether Sumit was still working on this bug or whether you still need a patch for it? If yes then I would like to have a go. I have cloned (using Mercurial) and built the latest UX branch and got it working on my Mac ok.

Comment 13

[Sumit Agrawal[:sumit4iit]]

Emma, I am not working on this bug presently. 
You can start on this bug.

Thanks :)

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Thanks Sumit - re-assigning to Emma.

Emma - I've received your email and will respond shortly.

Comment 15

[Emma Sajic]

Attachment: bug_885926_patch_1.patch

Thanks Sumit :-)

Mike thanks for your very helpful email about this and for getting back to me so quickly.

I decided to start again using the mozilla-central repository instead of the UX one because the Australis changes got merged into the main Nightly repository (mozilla-central) recently as you mentioned in your email.

My first attempt at a patch is attached here!

Here is info on what I did based on your very handy advice:
-------------------
1. Use Mercurial hg to check out the latest mozilla-central branch
hg clone http://hg.mozilla.org/mozilla-central/ src
cd src
2. Follow build process for Mac OS X using .mozconfig and ./mach build to build Firefox
3. Use ./mach run to run Firefox
4. In address bar type about:config and then set devtools.chrome.enabled to true
5. Tools --> Web Developer --> Scratchpad to launch Scratchpad, then from Scratchpad's Environment Menu select BROWSER rather than CONTENT to run in the Browser context.
6. Execute --> Run to run Scratchpad script (this script's use of CustomizableUI will only work on the Firefox you built yourself using ./mach build)
let widgetGroup = CustomizableUI.getWidget("sync-button");
let node = widgetGroup.forWindow(window).node;
node
node.id
7. If successful the script won't give any output. You need to highlight and use the DISPLAY button to print out the value of "node"
It should have the value [object XULElement] (it's a XULElement object).
8. Highlight and use the DISPLAY button to print out the value of "node.id" -- it should have the value "sync-button".

Additional buttons you can test in the same way as sync-button are:
 feed-button
 home-button
 downloads-button
 developer-button

9. Quit Firefox and run the customizableui test suite using ./mach mochitest-browser browser/components/customizableui/test  
This gives a benchmark of how the tests behave on the machine you're using (as no changes have been made to code yet).

10. Make bug fix edits to src/browser/components/customizableui/src/CustomizableUI.jsm and rebuild.

11. Re-run scratchpad tests and customizableui test suite to verify everything works ok.

12. Use Mercurial to generate a patch.
 hg qnew <patch file name>.patch 
To update the patch file with the latest changes you've made in the repository run hg qref 
Patch will be created in <repository>/.hg/patches
----------------

Feedback very welcome.

Cheers and thanks,
Emma

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8349584 [details] [diff] [review]
bug_885926_patch_1.patch

Putting into my review queue.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8349584 [details] [diff] [review]
bug_885926_patch_1.patch

Review of attachment 8349584 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good - but we should also remove the definition of idToSelector.

Thanks Emma!

Comment 18

[Emma Sajic]

Attachment: bug_885926_patch_1.patch

Cheers Mike :-)
Yes good point. I've made an improved patch with the obsolete idToSelector function removed (it was only being used within CustomizableUI.jsm before and I've taken out all references to it now in this fix anyway).
I've re-tested using the scratchpad and the customizableui test suite.
Thanks!
Emma

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8349613 [details] [diff] [review]
bug_885926_patch_1.patch

Review of attachment 8349613 [details] [diff] [review]:
-----------------------------------------------------------------

I like it! Thanks Emma!

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Landed on mozilla-inbound as https://hg.mozilla.org/integration/fx-team/rev/a97cb4ad5ba1

Comment 21

[Emma Sajic]

Cheers Mike thanks very much for all your help on this :-)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

(In reply to Emma Sajic from comment #21)
> Cheers Mike thanks very much for all your help on this :-)

Thank you for your thorough and thoughtful approach! Let me know if you want any help finding another bug to hack on.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a97cb4ad5ba1