Skip to content

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

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Fix recent perf regressions #188

merged 1 commit into from
Aug 22, 2019

Conversation

bicknellr
Copy link
Collaborator

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.

Copy link
Collaborator

@kevinpschaaf kevinpschaaf left a 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.

Copy link
Collaborator

@sorvell sorvell left a 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.

@bicknellr
Copy link
Collaborator Author

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)

___________________________________________________
_     Version _ <none>                            _
___________________________________________________
_     Browser _ edge                              _
_             _ 18.17763                          _
___________________________________________________
_ Sample size _ 500                               _
___________________________________________________

____________________________________________________________________________________________________________
_ Benchmark      _ Bytes     _          Avg time _ vs #0 (cf4d9550) _ vs #5 (f0a44fc3) _ vs fix (6f219d44) _
____________________________________________________________________________________________________________
_ #0 (cf4d9550)  _ 22.18 KiB _ 34.79ms - 35.02ms _                  _           faster _            faster _
_                _           _                   _         -        _          3% - 4% _           0% - 1% _
_                _           _                   _                  _  1.04ms - 1.37ms _    0.06ms - 0.4ms _
____________________________________________________________________________________________________________
_ #5 (f0a44fc3)  _ 24.81 KiB _ 35.99ms - 36.22ms _           slower _                  _            slower _
_                _           _                   _          3% - 4% _         -        _           2% - 3% _
_                _           _                   _  1.04ms - 1.37ms _                  _   0.80ms - 1.14ms _
____________________________________________________________________________________________________________
_ fix (6f219d44) _ 24.81 KiB _ 35.01ms - 35.26ms _           slower _           faster _                   _
_                _           _                   _          0% - 1% _          2% - 3% _          -        _
_                _           _                   _  0.06ms - 0.40ms _   0.8ms - 1.14ms _                   _
____________________________________________________________________________________________________________

___________________________________________________
_     Version _ <none>                            _
___________________________________________________
_     Browser _ edge                              _
_             _ 18.17763                          _
___________________________________________________
_ Sample size _ 500                               _
___________________________________________________

____________________________________________________________________________________________________________
_ Benchmark      _ Bytes     _          Avg time _ vs #0 (cf4d9550) _ vs #5 (f0a44fc3) _ vs fix (6f219d44) _
____________________________________________________________________________________________________________
_ #0 (cf4d9550)  _ 22.18 KiB _ 34.82ms - 35.01ms _                  _           faster _            faster _
_                _           _                   _         -        _          3% - 4% _           0% - 1% _
_                _           _                   _                  _    1.1ms - 1.4ms _      0ms - 0.29ms _
____________________________________________________________________________________________________________
_ #5 (f0a44fc3)  _ 24.81 KiB _ 36.06ms - 36.28ms _           slower _                  _            slower _
_                _           _                   _          3% - 4% _         -        _           3% - 4% _
_                _           _                   _  1.10ms - 1.40ms _                  _   0.95ms - 1.27ms _
____________________________________________________________________________________________________________
_ fix (6f219d44) _ 24.81 KiB _ 34.95ms - 35.17ms _           slower _           faster _                   _
_                _           _                   _          0% - 1% _          3% - 3% _          -        _
_                _           _                   _  0.00ms - 0.29ms _  0.95ms - 1.27ms _                   _
____________________________________________________________________________________________________________

The relative times between the commits are roughly the same as what I was seeing in Chrome.

@bicknellr bicknellr merged commit 005e5e7 into master Aug 22, 2019
@bicknellr bicknellr deleted the fix-perf-regressions branch August 22, 2019 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants