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

Use localName instead of nodeName #37

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Aug 8, 2020

Due to heave usage of getComputedStyle in jsdom we discovered that Element.prototype.nodeName was a bottleneck. It seems like this isn't the correct property to use for selectors but Element.prototype.localName (which is way cheaper in jsdom).

See:

It isn't clear to me how to test these changes extensively. I quickly verified it in ./test/jquery/jquery, ./test/html5/, ./test/slick/Runner/runner (all in Chrome 84) and by using this PR in jsdom's yarn test-wpt. All test suites still pass with these changes.

@dperini
Copy link
Owner

dperini commented Aug 11, 2020

@eps1lon
I am going to do the full testing and accept this change if it doesn't produce regressions.
Thank you for the suggestion.

@@ -534,7 +534,7 @@
// contentType not in IE <= 11
'contentType' in doc ?
doc.contentType.indexOf('/html') > 0 :
doc.createElement('DiV').nodeName == 'DIV';
doc.createElement('DiV').localName == 'DIV';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localName reflects the element name without ASCII upper‑casing, so it will be "DiV" and not "DIV".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ExE-Boss
Thank you for the help following with us.
In both Firefox and Chrome (latest versions):
document.createElement('DiV').localName == 'div'
instead of 'DiV'. Any thought ?

If this is true then `localName' is the lower-casing version of the tag name.

Recall that this differentiation was introduced to support mixed namespaces in the same document (SGV mostly).
Do you believe we are at risk of introducing regressions related to that multi-namespaces support with these changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we could just switch to

Suggested change
doc.createElement('DiV').localName == 'DIV';
doc.createElement('DiV').localName == 'div';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lowercasing of localName only happens in HTML documents:

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ExE-Boss
Ok, by reading that I understand that the functionality of localName depends on the document type.

@dperini
Copy link
Owner

dperini commented Aug 19, 2020

@eps1lon
the commit has been pushed, your suggestion about the test in isHTML() seems adequate.
I also removed two or three .toLowerCase() since they were not necessary any more.
Not much speed gain, but surely an improvement even in nwsapi speed itself.
Curious about the gain we should see in jsdom which is were this started.
Looking forward to comments and more improvements/adjustments.

@eps1lon
Copy link
Contributor Author

eps1lon commented Aug 20, 2020

Curious about the gain we should see in jsdom which is were this started.

We had pretty big gains in @testing-library/react in getComputedStyle heavy methods. We're now caching tagName (used by nodeName) in jsdom which lead to up to 40% reduced runtime. Caching tagName and nwsapi using localName are basically equivalent. jsdom/jsdom#3008 has more context (including a benchmark repo).

@eps1lon the commit has been pushed,

What commit? I see this PR now has conflicts. Should I resolve these and is there anything else I should be doing?

@dperini
Copy link
Owner

dperini commented Aug 21, 2020

@eps1lon
sorry for the mess ... I did things as I should have done from the beginning.
Accepted your pull request ... I will add the small difference in another commit.
After so many changes and error, I believe I would better do a git rebase too when all the new commits are in place.

@eps1lon
Copy link
Contributor Author

eps1lon commented Aug 21, 2020

No worries. Whatever makes your life easier.

@eps1lon eps1lon deleted the feat/perf-nodeName-localName branch May 15, 2021 11:46
dperini added a commit that referenced this pull request Jul 16, 2022
dperini added a commit that referenced this pull request Jul 31, 2022
…istake while attempting to restore lost case-sensitive regression
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

Successfully merging this pull request may close these issues.

None yet

3 participants