Assorted Performance Improvements #3041

kfule · 2025-08-17T13:23:08Z

Description

This PR improves performance through the following changes:

Adoption of the spread syntax, which can be optimized in modern browsers
Early return in execSelector() when attrs is null or undefined
- Returns ~~undefined or~~ the attrs generated by the selector directly
~~Allows attrs to be undefined, skipping lifecycle-related processing during rendering~~
Skips calling updateAttrs() entirely when the attrs generated by a selector (without form attributes) remain unchanged

In order to support spread syntax, the internal bundler has also been changed in this PR.

result of `npm run perf`

`npm run perf` (on my laptop, node v22)	v2.3.4	this PR	Performance Gain (%)
construct large vnode tree	20,876	26,040	24.74%
rerender identical vnode	4,460,814	4,738,273	6.22%
rerender same tree	90,246	103,905	15.14%
rerender same tree (with selector-generated attrs)	–	139,281	54.33%
add large nested tree	7,705	8,804	14.26%
mutate styles/properties	140	142	1.43%
repeated add/removal	2,950	2,973	0.78%

As shown above, the performance of “construct large vnode tree”, “rerender same tree”, and “add large nested tree” will improve.

I've confirmed performance improvements even in real browsers, but detailed benchmark measurements would take much time, so I've put them off for now. (I'll conduct and compile them separately if needed.)

Note that "rerender same tree (with selector-generated attrs)" is measured by changing the code as follows. Only the attribute part has been changed from the original code.

suite.add("rerender same tree", {
	fn: function () {
		m.render(rootElem, m(".foo.bar[data-foo=bar][p=2]",
			m("header",
				m("h1.asdf", "a ", "b", " c ", 0, " d"),
				m("nav",
					m("a[href='/foo']", "Foo"),
					m("a[href='/bar']", "Bar")
				)
			),
			m("main",
				m("form", {onSubmit: function () {}},
					m("input[type='checkbox'][checked]"),
					m("input[type='checkbox']"),
					m("fieldset",
						m("label",
							m("input[type=radio][checked]")
						),
						m("label",
							m("input[type=radio]")
						)
					),
					m("button-bar",
						m("button[style='width:10px; height:10px; border:1px solid #FFF;']",
							"Normal CSS"
						),
						m("button[style='top:0 ; right: 20']",
							"Poor CSS"
						),
						m("button[style='invalid-prop:1;padding:1px;font:12px/1.1 arial,sans-serif;'][icon]",
							"Poorer CSS"
						),
						m("button[style='margin: 0; padding: 10px; overflow: visible']",
							"Object CSS"
						)
					)
				)
			)
		))
	},
})

Motivation and Context

I've been interested in improving mithril's performance since last year, and have been experimenting through trial and error. These changes are among those that proved effective with relatively minor code changes.

How Has This Been Tested?

npm run test including new tests.

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.

The implementation adopts spread syntax, inspired by the approach mentioned in the comments. However, argument handling has been adjusted for better performance. Using spread syntax not only allows related comments to be deleted, but also slightly reduces bundle size and improves benchmarks by several percent. All existing tests pass successfully. Note that spread syntax does not seem to be supported by the existing internal bundler in some cases, so the bundler has also been updated to prevent incorrect suffix handling.

Since commit f9e5163, attrs is never null/undefined. However, `m.render()` takes into account the case where attrs is null/undefined, and it is better to use undefined from a performance point of view. Also, early return within `execSelector()` contributes to further performance improvement. Some tests are changed because of the null/undefined attrs. This commit may be breaking from this point of view, but the v2.0.4 era allows for attrs to be null/undefined and the impact will be quite small.

…rs are identical Since `compileSelector()` generates attrs objects containing only strings (no functions or nested objects). So, `updateAttrs()` can safely be skipped by checking the equality of the cached attrs objects themselves if the objects do not contain form attributes. If you mainly use the "static" attrs and do not use dynamic attrs, skipping `updateAttrs()` allows significant performance improvements. Also, the added checks are lightweight and there appears to be little or no performance degradation due to these checks. This commit itself is not a breaking change in the sense that it passes all existing tests. The checks in `m.render()` above and the form attribute checks in `compileSelector()` result in a minor increase in code volume, though.

This state.attrs.className != null check is redundant.

It seems that the current codebase (including generator, dom-for and spread syntax) already has little support for IE11 because it requires transpiling as well as polyfills.

dead-claudia

In addition to the other comments, I strongly would prefer existing behavior to remain intact, to minimize breakage. The tests shouldn't need changed for this PR.

render/cachedAttrsIsStaticMap.js

render/fragment.js

render/hyperscriptVnode.js

render/hyperscript.js

As this results in a slight decrease in performance improvement, comments regarding potential performance improvements have also been added.

render/hyperscript.js

This simplifies the processing within execSelector.

kfule · 2025-08-18T13:17:43Z

@dead-claudia Thank you for your kind comments.
I changed the implementation to be WeakMap-based and tried to address your other comments as well.

dead-claudia

One very minor nit and one request: could you run the perf numbers again?

Once those two happen, I'll approve it.

render/cachedAttrsIsStaticMap.js

This slightly reduces the bundle size.

kfule · 2025-08-23T02:27:39Z

@dead-claudia Thank you for your comment.
This is the current result of npm run perf. Performance has degraded compared to the initial PR, but it is still improved compared to v2.3.4.

`npm run perf` (on my laptop, node v22)	v2.3.4	this PR	Performance Gain (%)
construct large vnode tree	20,876	25,949	+24.30%
rerender identical vnode	4,460,814	4,677,364	+4.85%
rerender same tree	90,246	100,954	+11.87%
rerender same tree (with selector-generated attrs)	-	131,815	+46.06% (vs "rerender same tree" for v2.3.4)
add large nested tree	7,705	8,277	+7.42%
mutate styles/properties	140	139	-0.71%
repeated add/removal	2,950	2,949	-0.03%

dead-claudia · 2025-08-23T02:32:02Z

@kfule What is the perf result for "rerender same tree (with selector-generated attrs)" for v2.3.4?

kfule · 2025-08-23T02:43:24Z

@dead-claudia
It is 86,595 ops/sec. This was measured at the beginning of PR along with the results of that table.
In v2.3.4, it has somewhat worse than "rerender same tree".

* hyperscriptVnode: use spread syntax The implementation adopts spread syntax, inspired by the approach mentioned in the comments. However, argument handling has been adjusted for better performance. Using spread syntax not only allows related comments to be deleted, but also slightly reduces bundle size and improves benchmarks by several percent. All existing tests pass successfully. Note that spread syntax does not seem to be supported by the existing internal bundler in some cases, so the bundler has also been updated to prevent incorrect suffix handling. * allow attrs to be undefined and execute early return in `execSelector()` Since commit f9e5163, attrs is never null/undefined. However, `m.render()` takes into account the case where attrs is null/undefined, and it is better to use undefined from a performance point of view. Also, early return within `execSelector()` contributes to further performance improvement. Some tests are changed because of the null/undefined attrs. This commit may be breaking from this point of view, but the v2.0.4 era allows for attrs to be null/undefined and the impact will be quite small. * use the cached attrs as-is and skip `updateAttrs()` if the static attrs are identical Since `compileSelector()` generates attrs objects containing only strings (no functions or nested objects). So, `updateAttrs()` can safely be skipped by checking the equality of the cached attrs objects themselves if the objects do not contain form attributes. If you mainly use the "static" attrs and do not use dynamic attrs, skipping `updateAttrs()` allows significant performance improvements. Also, the added checks are lightweight and there appears to be little or no performance degradation due to these checks. This commit itself is not a breaking change in the sense that it passes all existing tests. The checks in `m.render()` above and the form attribute checks in `compileSelector()` result in a minor increase in code volume, though. * refactor execSelector() This state.attrs.className != null check is redundant. * README: drop IE11 support It seems that the current codebase (including generator, dom-for and spread syntax) already has little support for IE11 because it requires transpiling as well as polyfills. * prevents existing tests from being changed As this results in a slight decrease in performance improvement, comments regarding potential performance improvements have also been added. * use a common empty attrs object This simplifies the processing within execSelector. * cachedAttrsIsStaticMap: use WeakMap to avoid potential memory leaks * hyperscriptVnode: add performance sensitivity comments * cachedAttrsIsStaticMap: revert back to Map from WeakMap with comments * pass emptyAttrs to the Map constructor This slightly reduces the bundle size.

chadlwilson · 2025-08-27T07:31:34Z

I don't have a simple reprod yet, as haven't spent time looking into it, but FYI something regressed from this change.

Most of our test suite is full of errors in render.js during redraw() such as TypeError: can't access property "onremove" of undefined etc.

Firefox 142.0 (Mac OS 10.15) ERROR: TypeError: can't access property "onbeforeupdate" of undefined
shouldNotUpdate@webpack-internal:///./node_modules/mithril/render/render.js:865:41
updateNode@webpack-internal:///./node_modules/mithril/render/render.js:403:23
updateNodes@webpack-internal:///./node_modules/mithril/render/render.js:298:21
updateElement@webpack-internal:///./node_modules/mithril/render/render.js:461:15
updateNode@webpack-internal:///./node_modules/mithril/render/render.js:412:28
updateNodes@webpack-internal:///./node_modules/mithril/render/render.js:298:21
updateElement@webpack-internal:///./node_modules/mithril/render/render.js:461:15
updateNode@webpack-internal:///./node_modules/mithril/render/render.js:412:28
updateNodes@webpack-internal:///./node_modules/mithril/render/render.js:298:21
updateFragment@webpack-internal:///./node_modules/mithril/render/render.js:439:14
updateNode@webpack-internal:///./node_modules/mithril/render/render.js:411:30
updateComponent@webpack-internal:///./node_modules/mithril/render/render.js:471:19
updateNode@webpack-internal:///./node_modules/mithril/render/render.js:415:24
updateNodes@webpack-internal:///./node_modules/mithril/render/render.js:298:21
module.exports/<@webpack-internal:///./node_modules/mithril/render/render.js:907:15
sync@webpack-internal:///./node_modules/mithril/api/mount-redraw.js:12:16
TestHelper</TestHelper.prototype.redraw@webpack-internal:///./webpack/views/pages/spec/test_helper.tsx:319:30
TestHelper</TestHelper.prototype.mount@webpack-internal:///./webpack/views/pages/spec/test_helper.tsx:200:10
@webpack-internal:///./webpack/views/pages/new-environments/spec/environment_body_widget_spec.tsx:49:12

In case it's something due to the way our tests are written....
https://github.com/gocd/gocd/blob/b7d4df39effd555aeafb3492855f2ec0078bc283/server/src/main/webapp/WEB-INF/rails/webpack/views/pages/spec/test_helper.tsx

Seems something in MithrilJS/mithril.js#3041

dead-claudia · 2025-08-29T07:28:49Z

@chadlwilson Please file a new issue about this, so it's easier to track. You can link this PR in it.

chadlwilson · 2025-08-29T07:37:24Z

Understood, just wanted to double-check you're expecting zero regressions/behavioral changes from this before digging deeper.

dead-claudia · 2025-08-29T07:58:15Z

Understood, just wanted to double-check you're expecting zero regressions/behavioral changes from this before digging deeper.

We were not expecting regressions, that is correct. The only observable changes should be in property access order within m. Anything else is a regression.

kfule · 2025-08-29T11:52:59Z

shouldNotUpdate@webpack-internal:///./node_modules/mithril/render/render.js:865:41

The line causing the error appears to be the vnode.state line, not the vnode.attrs line.

mithril.js/render/render.js

Line 865 in c876faf

    
           if (typeof vnode.tag !== "string" && typeof vnode.state.onbeforeupdate === "function") {

This seems odd since this PR is not a change regarding vnode.state. Also, even if it were a vnode.attrs line, vnode.attrs should not be undefined. For vnode.attrs, I had put in a commit to make it nullable, but it was not merged.

chadlwilson · 2025-08-29T16:05:39Z

At a random guess, possibly because earlier it was breaking in the earlier clause while vnode.attrs is not null and has an onbeforeupdate, and not proceeding to the vnode.state check at all, but now vnode.attrs has become null.

If i revert the changes just to hyperscriptVnode.js (and invocations) the problem goes away.

Also goes away if I undo the bit from hyperscriptVnode.js from this commit: kfule@22adce1#diff-0fefa43cb559ae4535a2a313fa2fec8dc0122b127e850c911aaef7cb18241786L13-L14 - but leave the hyperscript.js changes, so essentially go back to the code where attrs is always {}, but cannot be null or undefined as originally commented by @dead-claudia

Will dig more tomorrow and raise a proper issue. I suspect that the earlier tests were missing a case that was important here.

kfule · 2025-08-30T00:45:15Z

@chadlwilson Thank you for your comment!
When reverting vnode.attrs to non-null (396dcca), I did not fully take into account the cases of m.fragment(m.Fragment) or component vnodes. This should be fixed along with the addition of test cases to avoid breaking previous behavior.
This may explain the following comment.

At a random guess, possibly because earlier it was breaking in the earlier clause while vnode.attrs is not null and has an onbeforeupdate, and not proceeding to the vnode.state check at all, but now vnode.attrs has become null.

I also want to confirm: is the error only occurring in the onbeforeupdate case? A previous comment also mentioned onremove.
#3041 (comment)

Most of our test suite is full of errors in render.js during redraw() such as TypeError: can't access property "onremove" of undefined etc.

I feel it's a bit odd that vnode.state is undefined during re-rendering when typeof vnode.tag !== "string".

In MithrilJS#3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

kfule · 2025-08-30T02:38:54Z

@chadlwilson I created #3042. Will this branch resolve the issue?

In MithrilJS#3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

chadlwilson · 2025-08-30T16:35:25Z

The branch/pr does seem to fix our unit tests, thanks! I didn’t get the chance to run our more full regression suite or raise a proper issue today, sorry. Will try again tomorrow 🙏🏻

chadlwilson · 2025-09-01T05:37:35Z

I raised a proper issue at #3043 - sorry for the delay.

In #3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

kfule added 5 commits August 17, 2025 19:51

refactor execSelector()

e6ee5d0

This state.attrs.className != null check is redundant.

README: drop IE11 support

e68b079

It seems that the current codebase (including generator, dom-for and spread syntax) already has little support for IE11 because it requires transpiling as well as polyfills.

kfule requested a review from a team as a code owner August 17, 2025 13:23

dead-claudia requested changes Aug 17, 2025

View reviewed changes

render/cachedAttrsIsStaticMap.js Outdated Show resolved Hide resolved

render/fragment.js Show resolved Hide resolved

render/hyperscriptVnode.js Show resolved Hide resolved

render/hyperscript.js Show resolved Hide resolved

render/hyperscript.js Show resolved Hide resolved

prevents existing tests from being changed

396dcca

As this results in a slight decrease in performance improvement, comments regarding potential performance improvements have also been added.

dead-claudia reviewed Aug 18, 2025

View reviewed changes

render/hyperscript.js Outdated Show resolved Hide resolved

render/hyperscript.js Show resolved Hide resolved

kfule force-pushed the perf-improve branch 2 times, most recently from b150220 to 5f1ae33 Compare August 18, 2025 08:27

kfule added 2 commits August 18, 2025 21:10

use a common empty attrs object

566b6d4

This simplifies the processing within execSelector.

cachedAttrsIsStaticMap: use WeakMap to avoid potential memory leaks

a3b18af

kfule force-pushed the perf-improve branch from 5f1ae33 to 31defdb Compare August 18, 2025 13:13

kfule requested a review from dead-claudia August 18, 2025 13:17

dead-claudia requested changes Aug 23, 2025

View reviewed changes

render/cachedAttrsIsStaticMap.js Outdated Show resolved Hide resolved

kfule added 3 commits August 23, 2025 10:44

hyperscriptVnode: add performance sensitivity comments

51a0598

cachedAttrsIsStaticMap: revert back to Map from WeakMap with comments

ac017b3

pass emptyAttrs to the Map constructor

7614401

This slightly reduces the bundle size.

kfule force-pushed the perf-improve branch from 31defdb to 7614401 Compare August 23, 2025 02:06

kfule requested a review from dead-claudia August 23, 2025 02:30

dead-claudia approved these changes Aug 25, 2025

View reviewed changes

dead-claudia merged commit cc0df94 into MithrilJS:main Aug 25, 2025
7 checks passed

JAForbes mentioned this pull request Aug 25, 2025

Release - v2.3.5 #3040

Merged

chadlwilson added a commit to gocd/gocd that referenced this pull request Aug 27, 2025

Revert to mithril 2.3.4 due to some kind of issue in 2.3.5

701aa25

Seems something in MithrilJS/mithril.js#3041

kfule added a commit to kfule/mithril.js that referenced this pull request Aug 30, 2025

Make the attrs of non-element vnodes always non-null.

1a5a1cc

In MithrilJS#3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

kfule mentioned this pull request Aug 30, 2025

Make the attrs of non-element vnodes always non-null. #3042

Merged

8 tasks

kfule added a commit to kfule/mithril.js that referenced this pull request Aug 30, 2025

Make the attrs of non-element vnodes always non-null.

3e0528a

In MithrilJS#3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

chadlwilson mentioned this pull request Sep 1, 2025

mithriljs 2.3.5 regresses behaviour for onbeforeupdate/onremove for fragments/children without attributes #3043

Closed

1 task

dead-claudia pushed a commit that referenced this pull request Sep 2, 2025

Make the attrs of non-element vnodes always non-null.

65c4229

In #3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

JAForbes mentioned this pull request Sep 2, 2025

Release - v2.3.6 #3044

Merged

dead-claudia pushed a commit that referenced this pull request Sep 2, 2025

Make the attrs of non-element vnodes always non-null.

2b740df

In #3041, it seemed that the case of non-element vnodes was not fully considered in terms of not breaking existing behavior.

This was referenced Oct 6, 2025

Performance Improvements for Vnode.normalizeChildren() #3051

Closed

refactor Vnode.normalizeChildren #3052

Merged

kfule deleted the perf-improve branch October 17, 2025 12:20

Uh oh!

Assorted Performance Improvements #3041

Assorted Performance Improvements #3041

Uh oh!

Conversation

kfule commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

result of npm run perf

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist

Uh oh!

dead-claudia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfule commented Aug 18, 2025

Uh oh!

dead-claudia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kfule commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dead-claudia commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfule commented Aug 23, 2025

Uh oh!

Uh oh!

chadlwilson commented Aug 27, 2025

Uh oh!

dead-claudia commented Aug 29, 2025

Uh oh!

chadlwilson commented Aug 29, 2025

Uh oh!

dead-claudia commented Aug 29, 2025

Uh oh!

kfule commented Aug 29, 2025

Uh oh!

chadlwilson commented Aug 29, 2025

Uh oh!

kfule commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfule commented Aug 30, 2025

Uh oh!

chadlwilson commented Aug 30, 2025

Uh oh!

chadlwilson commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kfule commented Aug 17, 2025 •

edited

Loading

result of `npm run perf`

kfule commented Aug 23, 2025 •

edited

Loading

dead-claudia commented Aug 23, 2025 •

edited

Loading

kfule commented Aug 30, 2025 •

edited

Loading