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

[refactor] Refactoring of hyperscript.js and render.js, including performance improvements #2983

Merged
merged 9 commits into from
Oct 31, 2024

Conversation

kfule
Copy link
Contributor

@kfule kfule commented Oct 21, 2024

Refactor hyperscript.js and render.js. In particular, the replacement of fix #2622 appears to have significantly improved the performance regression.

Description

This change is a refactoring aimed at improving code outlook and performance.

results of `npm run perf`

before (v2.2.8)

construct large vnode tree x 18,987 ops/sec ±0.79% (126 runs sampled)
rerender identical vnode x 3,898,180 ops/sec ±0.52% (127 runs sampled)
rerender same tree x 55,444 ops/sec ±1.71% (128 runs sampled)
add large nested tree x 6,701 ops/sec ±1.22% (123 runs sampled)
mutate styles/properties x 78.77 ops/sec ±3.08% (75 runs sampled)
repeated add/removal x 3,052 ops/sec ±1.15% (126 runs sampled)
Completed perf tests in 60100ms

after ("rerender same tree" is improved)

construct large vnode tree x 19,972 ops/sec ±0.81% (123 runs sampled)
rerender identical vnode x 3,889,751 ops/sec ±1.03% (127 runs sampled)
rerender same tree x 88,338 ops/sec ±0.27% (125 runs sampled)
add large nested tree x 7,014 ops/sec ±1.61% (119 runs sampled)
mutate styles/properties x 77.55 ops/sec ±1.12% (73 runs sampled)
repeated add/removal x 3,067 ops/sec ±2.48% (90 runs sampled)
Completed perf tests in 59057ms

cf. v2.0.4 (by backported test-perf.js and m.censor)

construct large vnode tree x 13,149 ops/sec ±0.51% (124 runs sampled)
rerender identical vnode x 5,084,840 ops/sec ±0.49% (127 runs sampled)
rerender same tree x 82,781 ops/sec ±0.18% (128 runs sampled)
add large nested tree x 6,611 ops/sec ±2.56% (101 runs sampled)
mutate styles/properties x 79.76 ops/sec ±0.86% (73 runs sampled)
repeated add/removal x 3,067 ops/sec ±0.97% (125 runs sampled)
Completed perf tests in 59579ms

As shown above, npm run perf is improved, especially for "rerender same tree".

I have also run test-perf.js in a real browser to see the performance improvement.
(If necessary, I would like to measure and tabulate performance later.)

Motivation and Context

As for performance, it is sufficiently sophisticated at v2.0.4. However, subsequent bug fixes and enhancements seem to have caused some performance regression. I measured the performance of v2.0.4 and later versions with npm run perf and found that 7c7d76d of #2578 has a negative impact on performance.
The commit 7c7d76d fixes #2622 by changing setAttr(s), but I noticed that it can be resolved by hyperscript(execSelector) instead of setAttr(s).

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My change requires a documentation update, and I've opened a pull request to update it already:
  • I have read https://mithril.js.org/contributing.html.

@kfule kfule requested a review from a team as a code owner October 21, 2024 17:03
@kfule kfule mentioned this pull request Oct 21, 2024
8 tasks
kfule added 3 commits October 27, 2024 00:26
… render.js with another workaround in hyperscript.js

The input[type] inspection at the beginning of setAttr() was called for each attribute. This had a negative impact on performance. The new workaround in execSelector() controls the order of setting attributes by reordering the keys in attrs.
The isFileInput is needed only when key is "value", so moving the logic into setAttr() would not increase time of calculation. Also, the code outlook improves a bit, of course.
The polyfill doesn't support multiple source objects, and almost all browsers now natively support Object.assign().
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

One minor nit, then it's got my approval.

(This will cause some merge conflicts for my #2982, but it wouldn't be the first time I rebuilt that PR.)

util/assign.js Outdated
if (hasOwn.call(source, key)) target[key] = source[key]
}
}
module.exports = Object.assign
Copy link
Member

Choose a reason for hiding this comment

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

Could you just drop this module entirely and use Object.assign directly in the places that use it?

Shouldn't impact performance, but will provide a small bundle size win and it's one less source file to worry about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I removed the module.

@dead-claudia
Copy link
Member

@kfule Sorry for the delayed review. Been hard at work nailing down a long math-heavy justification for one of the internal workflow changes in an upcoming update to #2982.

@kfule
Copy link
Contributor Author

kfule commented Oct 30, 2024

@dead-claudia
Thanks for your review.
I removed the assign.js module as you suggested.
Also, I removed unnecessary parameters from setAttr().

@kfule kfule requested a review from dead-claudia October 30, 2024 09:47
@dead-claudia dead-claudia merged commit 3c6c1cf into MithrilJS:main Oct 31, 2024
7 checks passed
@dead-claudia
Copy link
Member

dead-claudia commented Oct 31, 2024

Oops, meant to squash that, but was also debugging Refined GitHub at the same time. 🙃

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.

Error when using CSS selector for input type and passing a special attribute
2 participants