[refactor] Performance improvement of updateStyle()

[kfule]

This is a refactoring to improve the performance of updateStyle().

Description

In this PR, style.setProperty() in updateStyle() is used only when there is a dash "-" in the key. This seems to improve performance and reduce code size by eliminating normalizeKey() and toLowerCase().

The benchmark "mutate styles/properties" on nodejs show about an over 50% improvement as follows.

results of `npm run perf`

before (v2.2.9)

construct large vnode tree x 19,407 ops/sec ±0.66% (124 runs sampled)
rerender identical vnode x 3,853,625 ops/sec ±0.39% (128 runs sampled)
rerender same tree x 89,246 ops/sec ±0.15% (126 runs sampled)
add large nested tree x 7,300 ops/sec ±1.69% (120 runs sampled)
mutate styles/properties x 76.49 ops/sec ±0.93% (72 runs sampled)
repeated add/removal x 3,065 ops/sec ±2.28% (102 runs sampled)
Completed perf tests in 60285ms

after ("mutate styles/properties" is improved)

construct large vnode tree x 19,319 ops/sec ±0.61% (125 runs sampled)
rerender identical vnode x 3,999,668 ops/sec ±1.25% (129 runs sampled)
rerender same tree x 89,959 ops/sec ±0.30% (124 runs sampled)
add large nested tree x 7,337 ops/sec ±1.63% (122 runs sampled)
mutate styles/properties x 121 ops/sec ±1.35% (90 runs sampled)
repeated add/removal x 3,078 ops/sec ±2.93% (83 runs sampled)
Completed perf tests in 59106ms

I have not measured it precisely, but also in real browsers the benchmark “mutate styles/properties” seems to show an improvement (about 10%?).
(Edit) results in real browsers: #2985 (comment)

Motivation and Context

Following #2983, I backported the latest benchmark scripts to v1.1.7, measured and compared the benchmarks with v2, and found that the style-related benchmark "mutate styles/properties" were significantly slower. Even though the many fixes and enhancements merged into v2, its performance seemed too slow.

I checked the code and one of the major causes seemed to be the use of regular expressions and replacements (i.e. normalizeKey() and toLowerCase()) in updateStyle() to always use style.setProperty() to support css vars.

How Has This Been Tested?

• npm run test
  • The cssFloat normalization test has been removed in this PR as it is no longer needed.

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.

[dead-claudia]

@kfule Can you run the benchmarks in the browser using the internal test page in /performance/index.html? The cost of DOM property updates in the browser is considerably higher (between FFI overhead and the fact every string is copied in every browser before being processed further), so I'd like a better idea of how this plays out in the real world.

You can spin up any server you'd like in the repo's root and just open that page. Benchmark results will be printed in the web console.

[dead-claudia]

If it works as advertised, I also want to port it over to #2982, as that is the benchmark it's bottlenecked on most.

[dead-claudia]

Approved modulo nit + in-browser benchmark run request.

[kfule]

@dead-claudia
Thanks for the review. I will try to measure it in detail in the actual browser.
I just started measuring, and as you guessed, performance does not seem to improve in Firefox (x64,win) at first glance. On the other hand, it appears to be improving in Edge.
For now, I'll share the results of the measurements with Firefox(x64,win), Edge(x64,win) and Safari(mac,m2).

[kfule]

@dead-claudia
I measured performance on real browsers using the internal test page in /performance/index.html, so the results are shown below for Firefox (x64,win), Edge (x64,win), and Safari (m2, macOS).
Because the results change from run to run, each condition was measured five times, and the three intermediate values were averaged excluding the minimum and maximum values.

Summary

In Edge and Safari, the benchmark seems to have improved by about 10%.
In Firefox, as commented earlier, the first measurement showed no improvement. However, after changing indexOf() to includes() and measuring again under all conditions, the benchmark results, including indexOf(), improved.

Apart from the unclear results in Firefox, performance seems to have improved in Edge and Safari.

Details

Firefox 132.0 (x64,win) (1st round, no improvements)

PR's “mutate styles/properties” performance is about 102% vs. v2.2.9.

results

before(v2.2.9)	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	11,413	11,355	11,370	11,968	12,215		12,215	11,355	11583.66667	100
rerender identical vnode	2,059,720	1,371,061	1,431,168	1,400,981	1,971,213		2,059,720	1,371,061	1601120.667	100
rerender same tree	40,718	39,845	43,777	39,785	40,841		43,777	39,785	40468	100
add large nested tree	4,073	4,383	4,136	4,428	3,946		4,428	3,946	4197.333333	100
mutate styles/properties	43.53	39.34	43.05	43.01	44.24		44.24	39.34	43.19666667	100
repeated add/removal	5,818	5,427	5,738	5,470	5,605		5,818	5,427	5604.333333	100

after(indexOf())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	13,692	11,283	11,381	11,215	12,521		13,692	11,215	11728.33333	101.2488849
rerender identical vnode	1,724,405	1,653,268	1,716,446	1,406,369	1,376,909		1,724,405	1,376,909	1592027.667	99.43208528
rerender same tree	42,968	41,106	41,402	41,093	41,604		42,968	41,093	41370.66667	102.230569
add large nested tree	4,070	4,229	4,444	4,531	4,312		4,531	4,070	4328.333333	103.1210292
mutate styles/properties	45.94	44.34	44.04	44.11	43.79		45.94	43.79	44.16333333	102.237827
repeated add/removal	5,472	5,559	5,570	5,374	5,578		5,578	5,374	5533.666667	98.73907096

Edge 130.0 (x64,win)

PR's “mutate styles/properties” performance is about 114% vs. v2.2.9.

results

before(v2.2.9)	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	23,220	21,815	21,467	21,995	22,192		23,220	21,467	22000.66667	100
rerender identical vnode	2,726,283	2,505,887	2,697,283	2,645,936	2,591,252		2,726,283	2,505,887	2644823.667	100
rerender same tree	88,176	86,528	85,428	86,613	85,781		88,176	85,428	86307.33333	100
add large nested tree	9,738	9,519	9,668	9,377	9,709		9,738	9,377	9632	100
mutate styles/properties	53.62	53.69	52.65	52.9	52.39		53.69	52.39	53.05666667	100
repeated add/removal	2,518	2,888	2,934	2,519	1,746		2,934	1,746	2641.666667	100

after(indexOf())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	22,223	20,751	22,067	21,356	22,170		22,223	20,751	21864.33333	99.38032181
rerender identical vnode	2,742,269	2,678,420	2,665,716	2,642,867	2,606,596		2,742,269	2,606,596	2662334.333	100.6620731
rerender same tree	88,269	84,879	88,648	86,369	87,757		88,648	84,879	87465	101.3413306
add large nested tree	9,354	9,778	9,409	9,258	10,047		10,047	9,258	9513.666667	98.77145626
mutate styles/properties	63.21	60.33	59.79	60.82	61.2		63.21	59.79	60.78333333	114.5630458
repeated add/removal	2,864	1,969	2,931	1,611	2,802		2,931	1,611	2545	96.34069401

The results of includes() are measured later.

after(includes())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	21,036	22,327	21,638	21,814	21,930		22,327	21,036	21794	99.06063453
rerender identical vnode	2,583,059	2,624,186	2,595,332	2,636,622	2,739,703		2,739,703	2,583,059	2618713.333	99.01277603
rerender same tree	84,165	83,743	85,720	83,801	84,799		85,720	83,743	84255	97.62206379
add large nested tree	9,372	9,282	9,438	9,535	9,401		9,535	9,282	9403.666667	97.62942968
mutate styles/properties	60.9	60.35	60.31	61.37	61.44		61.44	60.31	60.87333333	114.7326758
repeated add/removal	2,556	3,000	2,931	1,232	2,981		3,000	1,232	2822.666667	106.851735

safari 17.4(m2,macOS)

PR's “mutate styles/properties” performance is about 108~111% vs. v2.2.9.

results

before(v2.2.9)	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	19,296	19,258	19,301	19,325	19,254		19,325	19,254	19285	100
rerender identical vnode	7,269,015	7,229,244	7,285,360	7,222,580	7,185,731		7,285,360	7,185,731	7240279.667	100
rerender same tree	99,898	100,998	100,557	100,937	100,815		100,998	99,898	100769.6667	100
add large nested tree	8,351	8,377	8,350	8,410	8,378		8,410	8,350	8368.666667	100
mutate styles/properties	193	193	195	194	195		195.00	193.00	194	100
repeated add/removal	26,146	26,112	25,837	26,407	25,714		26,407	25,714	26031.66667	100

after(indexOf())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	20,381	19,173	19,093	19,182	19,191		20,381	19,093	19182	99.46590614
rerender identical vnode	7,952,754	7,180,221	7,175,053	7,258,506	7,113,911		7,952,754	7,113,911	7204593.333	99.50711388
rerender same tree	103,422	100,631	100,502	99,585	99,491		103,422	99,491	100239.3333	99.47371729
add large nested tree	8,519	8,387	8,329	8,349	8,336		8,519	8,329	8357.333333	99.86457421
mutate styles/properties	222	217	214	215	216		222.00	214.00	216	111.3402062
repeated add/removal	27,517	25,918	26,824	25,748	26,136		27,517	25,748	26292.66667	101.002625

The results of includes() are measured later.

after(includes())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	19,519	19,527	19,646	19,551	19,591		19,646	19,519	19556.33333	101.4069657
rerender identical vnode	7,319,978	7,290,709	7,273,121	7,274,118	7,273,168		7,319,978	7,273,121	7279331.667	100.5393714
rerender same tree	101,098	101,148	101,077	100,799	101,162		101,162	100,799	101107.6667	100.3354184
add large nested tree	8,379	8,354	8,372	8,346	8,354		8,379	8,346	8360	99.8964391
mutate styles/properties	215	210	212	212	209		215.00	209.00	211.3333333	108.9347079
repeated add/removal	26,914	25,943	26,318	26,120	26,331		26,914	25,943	26256.33333	100.8630514

Firefox 132.0 (x64,win) (2nd round)

PR's “mutate styles/properties” performance is about 111~118% vs. v2.2.9.

results

before(v2.2.9)	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	9,781	9,365	11,219	11,596	11,666		11,666	9,365	10865.33333	100
rerender identical vnode	1,742,029	1,634,838	1,356,398	1,341,494	1,720,874		1,742,029	1,341,494	1570703.333	100
rerender same tree	40,947	45,169	40,562	40,374	40,865		45,169	40,374	40791.33333	100
add large nested tree	4,211	4,023	4,148	4,244	4,093		4,244	4,023	4150.666667	100
mutate styles/properties	40.99	43.92	39.56	42.97	44.93		44.93	39.56	42.62666667	100
repeated add/removal	5,435	5,453	5,262	5,323	5,622		5,622	5,262	5403.666667	100

after(indexOf())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	11,448	11,448	11,008	11,445	11,113		11,448	11,008	11335.33333	104.3256841
rerender identical vnode	1,829,853	1,358,577	1,589,985	1,699,296	1,730,162		1,829,853	1,358,577	1673147.667	106.5221949
rerender same tree	41,380	40,662	40,629	41,430	41,015		41,430	40,629	41019	100.5581251
add large nested tree	3,930	4,247	4,108	4,189	4,625		4,625	3,930	4181.333333	100.7388371
mutate styles/properties	49.14	49.46	43.53	44	49.59		49.59	43.53	47.53333333	111.5107914
repeated add/removal	5,610	5,542	5,110	5,137	5,746		5,746	5,110	5429.666667	100.4811548

after(includes())	1	2	3	4	5		max	min	average	vs v2.2.9(%)
construct large vnode tree	10,077	11,675	11,567	11,557	11,592		11,675	10,077	11572	106.5038655
rerender identical vnode	1,682,281	1,761,743	1,643,334	1,718,136	1,349,606		1,761,743	1,349,606	1681250.333	107.0380573
rerender same tree	40,963	42,377	41,703	42,280	41,183		42,377	40,963	41722	102.2815304
add large nested tree	4,672	3,980	4,253	4,287	4,217		4,672	3,980	4252.333333	102.4494057
mutate styles/properties	50.21	49.58	50.23	51.27	50.6		51.27	49.58	50.34666667	118.1107288
repeated add/removal	5,638	5,837	5,560	5,677	5,879		5,879	5,560	5717.333333	105.8047005

[kfule]

Also, /performance/index.html was executed by opening a local file in a browser without starting the server.
If hosting from a server is required, I would like to measure this later.

Edit: I hosted the files on the server and access to them by client browser as a test, but the results did not seem much different than if I had opened it locally.
Also, I ran it again in Firefox and it seems that sometimes the PR looks clearly better (about 10%) and other times I can't see any difference.

[dead-claudia]

@kfule To be clear, my .includes request was more of a style request. Perf-wise, any serious deviation would be an engine bug. Also, server vs file shouldn't matter for the benchmarks, so I wouldn't expect much difference.

Though...I decided to do a bit more reading, and turns out that the CSSStyleDeclaration interface also supports stuff like elem.style["font-size"] as well as the equivalent elem.style.fontSize. These dashed properties are required by spec to include every single non-custom (custom being --foo: bar) property the browser supports, including vendor-prefixed properties. This may require some DOM mock updates (you'll need to test for it), but what this means is you can change your condition to key.startsWith("--") and probably get even more performance since it's a branch that's pretty much always taken. Since prefixed properties are relatively rare in inline styles (way more than in stylesheets), you could probably even reduce that to key[0] === "-" as your condition and make the check trivial.

[dead-claudia]

Sorry for dragging this out for so long. I just want to make sure we're getting it right the first time so we don't have to come back to it later. 😅

[kfule]

@dead-claudia Thanks for your detailed comments. I don't mind that it takes time to kindly review.

So, there is no problem with if statements limited to custom properties, and there is room for performance improvement.
Then it seems to me that it would be better to use key[0] === "-" && key[1] === "-" taking the previous code and your suggestion into account. I think just key[0] === "-" would be fine, but it might be unclear what it is intended for.

If the change does not affect performance improvement, I would like to push it.

---

Also, in case you're interested, I've tried other approaches and will share my findings.

• The approach of caching the result of normalizeKey() does not seem to improve as much as PR for the complexity of the code.
• The method of using regular expressions to detect camelCase instead of includes() did not improve performance, nor did it take into account what would happen if the css vars contained uppercase letters.

[kfule]

includes("-") was changed to key[0] === "-" && key[1] === "-". There seems to be no negative impact on performance.

---

This may require some DOM mock updates (you'll need to test for it),

Continued use of dashed and camelCased properties did not seem to be supported by the current version of mithril as well as the DOM mock...

It may be better to add a set-get test for dashed properties (probably testable with current DOM mock), but the interoperability of dashed and camelCased properties may be handled separately from this PR .

Edit: The flems above appears that different keys operate on the same property, so after setting with a new key, the old different key deletes the new value.
This may be a minor bug that can be fixed by changing the order of set and delete.

[dead-claudia]

[...] but the interoperability of dashed and camelCased properties may be handled separately from this PR .

@kfule Resolving this would almost certainly tank attribute setting performance. I'd be open to adding that to https://github.com/MithrilJS/docs as a thing to watch out for, but I'm highly skeptical a library fix is viable.

[kfule]

@dead-claudia Thanks for approving.
The v2.2.10 was also released, but there is an error in Actions and it does not seem to be published to npm.
Could you please check this?
https://github.com/MithrilJS/mithril.js/actions/runs/11721294986

[dead-claudia]

That's so strange, considering it worked only a week ago.

[kfule]

@dead-claudia
The log shows a message GH013: Repository rule violations.
https://github.com/MithrilJS/mithril.js/actions/runs/11721294986/job/32648415996#step:7:21

There appears to be a rule in the release branch ruleset that prohibit force pushes.
https://github.com/MithrilJS/mithril.js/rules/1990808

[dead-claudia]

@kfule Yes, but v2.2.9 was successfully published and nobody made any changes between then and now. That's what makes this so confusing. In any case, I've manually completed the process.