Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace getElementsByTagName(tag)[0] with querySelector(tag) #6380

Closed
alvaromontoro opened this issue Feb 29, 2020 · 7 comments
Closed

Replace getElementsByTagName(tag)[0] with querySelector(tag) #6380

alvaromontoro opened this issue Feb 29, 2020 · 7 comments
Labels
external contributors welcome contribution is welcome! type: optimization code and performance optimizations

Comments

@alvaromontoro
Copy link

This is not a problem, it would be more of a refactoring/performance improvement task.

In different parts of the repo, we can find the function getElementsByTagName used in this way:

document.getElementsByTagName('head')[0]....

Using getElementsByTagName in such a way would be inefficient as it is searching the whole document looking for the tag (in the problematic cases "head" or "body" which should only appear once in the page), creating a list, and then accessing only the first element of that list. While querySelector would return the first element that fulfills the selector.

The suggestion would be, instead of doing:

document.getElementsByTagName('head')[0]....

we could do this:

document.querySelector('head')....

I considered that not using querySelector was due to the project aiming for some specific browser support. But there are other parts of the codebase which usequerySelector.

Here is a list of all the places where getElementsByTagName is used. Notice that not all of them would necessarily require this change.

@alvaromontoro
Copy link
Author

I'd be happy to pick up and complete this change if considered appropriate.

@rhymes rhymes added tech: javascript type: optimization code and performance optimizations labels Feb 29, 2020
@rhymes
Copy link
Contributor

rhymes commented Feb 29, 2020

Hi @alvaromontoro, thanks a lot for raising the issue. JS selectors performance is always an odd mishmash of different results.

I created an example on jsperf.com and ran it with Firefox 74:

Screenshot_2020-02-29 getElementsByTagName 0  vs querySelector · jsPerf

and Chrome 80:

jsperf com_getelementsbytagname-0-vs-queryselector-2

In both browser we can see that document.head is way faster than querySelector("head") which as you indicated it's also faster than getElementsByTagName("head")[0].

I imagine is the same for document.body over the other two.

When the selector is a generic tag though, things change a little.

Firefox 74:

Screenshot_2020-02-29 getElementsByTagName 0  vs querySelector · jsPerf(1)

Chrome 80:

jsperf com_getelementsbytagname-0-vs-queryselector-2_1

So, with superficial benchmarking, seems that getElementsByTagName is faster than querySelector

For curiosity I also tried with getElementsByTagName vs querySelectorAll with 5 links in a div and the formers seems to be much faster than the latter.

Firefox 74:

Screenshot_2020-02-29 getElementsByTagName 0  vs querySelector · jsPerf(2)

Chrome 80:

jsperf com_getelementsbytagname-0-vs-queryselector-2 (1)

I search on the internet to find out if these numbers are real and found this article from 2010: Why is getElementsByTagName() faster than querySelectorAll()?

It explains that getElementsByTagName is faster because it returns a live list of the nodes (if you cache the result in a variable, add a node that matches the query, getElementsByTagName will include the new node as well), instead querySelector(All) returns a static list of nodes. There's a simple snippet that explains it here: https://stackoverflow.com/a/30921553/4186181

The article from 2010 explains it like this:

Live NodeList objects can be created and returned faster by the browser because they don’t have to have all of the information up front while static NodeLists need to have all of their data from the start.

Basically the underlying data structure is more costly to build in the case of querySelector(All).

My suggestion would be to go in this order:

  1. replace the getElementsByTagName for head with document.head if possible, same for body
  2. keep getElementsByTagName("tag")[0] in the other cases

I learned a lot researching this @alvaromontoro, thank you!

@alvaromontoro
Copy link
Author

@rhymes thank you for sharing the research. It is really interesting (document.head being 10 times faster is mind-blowing). Should I proceed then with a PR or close the ticket?

@rhymes
Copy link
Contributor

rhymes commented Feb 29, 2020

@alvaromontoro please do! We've learned how faster document.head|body is, why not put this information to good use! Thank you!

@rhymes rhymes added the external contributors welcome contribution is welcome! label Feb 29, 2020
@icncsx
Copy link
Contributor

icncsx commented Jun 5, 2020

@rhymes is this issue stale? I can go ahead and make the PR if no one else will :)

@rhymes
Copy link
Contributor

rhymes commented Jun 7, 2020

@icncsx thank you! I'll take a look!

@rhymes
Copy link
Contributor

rhymes commented Oct 1, 2020

I'm going to close this as I think it has been addressed by #8315 - Let me know otherwise @alvaromontoro and @icncsx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external contributors welcome contribution is welcome! type: optimization code and performance optimizations
Projects
None yet
Development

No branches or pull requests

3 participants