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

document.querySelectorAll sluggish on Firefox (v30) for large DOM #629

Closed
jcmoore opened this issue Jul 11, 2014 · 9 comments
Closed

document.querySelectorAll sluggish on Firefox (v30) for large DOM #629

jcmoore opened this issue Jul 11, 2014 · 9 comments
Assignees

Comments

@jcmoore
Copy link
Contributor

jcmoore commented Jul 11, 2014

On Firefox v30, the platform.js "wrapped" versions of querySelectorAll, getElementsByClassName, getElementsByTagName, and getElementsByName take much longer to execute than their "unwrapped" counterparts.

Example page can be found here -- https://gist.github.com/jcmoore/44b117ce6cd6ab4630f3

@arv
Copy link
Contributor

arv commented Jul 15, 2014

This is an unfortunate consequence of how we do the ShadowDOM polyfill.

What is worse is that we cannot do live node lists so getElementsByClassName, getElementsByTagName and getElementsByName traverse the whole document instead of exiting when first element is found.

@arv arv closed this as completed Jul 15, 2014
@jcmoore
Copy link
Contributor Author

jcmoore commented Jul 17, 2014

Thank you for the swift response @arv . Admittedly, my understanding of all of the inner-workings of platform.js is naive at best, but any chance you (or someone) can elaborate on why the wrapped NodeLists are built from the bottom-up (by javascript tree traversal), instead of from the top-down (by filtering native NodeLists generated by calls to the native querying methods)?

I have an example that seems to work well in Firefox here -- https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1

specifically, please see:
https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector-js-L11-L40
https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector-js-L110-L115

and likewise changes to the other overridden query functions:
https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector-js-L100-L105
https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector-js-L123-L128
https://gist.github.com/jcmoore/69a49f887e03c1d2bfa1#file-queryselector-js-L146-L151

From platform.js' perspective, do the overridden query methods have to do anything beyond keeping "ShadowDOM" elements out of the resulting (wrapped) NodeLists? Is there something else I'm missing?

Thanks in advance,

@arv
Copy link
Contributor

arv commented Jul 17, 2014

That is a great approach. I don't see any problems with it and it should add a much needed boost.

At one point we didn't have the treeScope_ so a full walk up the tree would have been required.

We have open bugs on supporting :shadow and other new selectors here but we can special case them.

Do you mind sending a PR?

@arv arv reopened this Jul 17, 2014
jcmoore added a commit to jcmoore/ShadowDOM that referenced this issue Jul 17, 2014
@jcmoore
Copy link
Contributor Author

jcmoore commented Jul 17, 2014

Sure thing, but it may take a little while (as I need to get authorization from my employer to agree on the CLA...)

@arv
Copy link
Contributor

arv commented Jul 18, 2014

I thought of a problem with using this approach.

<a>
  <b></b>
</a>
var a = ...
var sr = a.createShadowRoot();
sr.innerHTML = 'no content element';
assert.equal(a.firstElementChild, a.querySelector('b'));

The problem is that <b> is not in the composed tree since <a> does not have a <content>.

We might still want to do this even though it breaks a few edge cases. The current solution breaks a few other cases (like 'a > b' when b is composed into some other parent).

@jmesserly

@arv
Copy link
Contributor

arv commented Jul 24, 2014

@jcmoore You can disable the tests but we do not want to commit breaking tests.

@ebidel
Copy link
Contributor

ebidel commented Jul 30, 2014

This is awesome @jcmoore. It would be killer to have some before and after numbers to point people to when all of this lands.

@sorvell
Copy link
Contributor

sorvell commented Aug 12, 2014

@ebidel This has now been integrated (yay!) and it sounds like we can close the issue. Leaving it open for you to review since you asked for some data.

@ebidel
Copy link
Contributor

ebidel commented Aug 12, 2014

Anyone have decent numbers to share? If not we can close. I just thought it would be great to show perf progress.

@sorvell sorvell closed this as completed Aug 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants