-
Notifications
You must be signed in to change notification settings - Fork 169
Fix recent perf regressions #188
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
Conversation
…ing unflushed definitions by name only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the change in the map code looks equivalent. Somewhat surprising that iterating the array of names shows up as meaningfully faster than a forEach over the map (which iiuc requires a key sort) given the benchmark used for testing wasn't taxing a large number of element tag names; but fine for now and we can do another round of building out benchmarks and micro-opting later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on the code, but let's make sure to test performance on Edge and IE (the main browsers where the polyfill is needed) before landing this.
Here's two different runs on Edge that benchmark walking a pseudo-random tree of 8192 nodes with ~50% trivial custom elements. (cf4d955 is before the PRs we were concerned about, f0a44fc is with those PRs, 6f219d4 is with this PR also)
The relative times between the commits are roughly the same as what I was seeing in Chrome. |
Mitigates perf regressions introduced in some recent PRs by removing the
namespaceURI
check (externally observable, but never released) and storing unflushed definitions by name only.