Improve handling of is-elements and Fix tiny bugs of setAttr()/updateStyle()

[kfule]

Fixes a few tiny bugs in attributes and style properties updates, and improves handling of is-elements in updateNode().

Description

• removes key === "is" from setAttr() to fix 2799 Custom elements: built-in element extensions lose their is attribute #2799 (comment)
  • Also, for code consistency, the key === "is" is removed from removeAttr() as well.
  • In addition, is-elements handling in updateNode() has been improved.
• swaps set and removal order in updateAttrs()/updateStyle() to prevent accidental removal for newly setting values
• removes cssText property access as Bypass css text #2811 .

Motivation and Context

I found a strange behavior in the process of #2985, although it rarely happens (#2985 (comment)). And there seems to be a similar trivial issue in updateAttrs() (flems.)
This could be solved by simply swapping the order of set and removal probably without creating new bugs and perf issues.

Also, I am not very familiar with custom elements, but it seemed to me that the #2799 issue could be solved by simply removing key === "is" from setAttr().

How Has This Been Tested?

• npm run test including test code fix for custom elements
  • Sorry, but I did not make any changes to domMock because the changes seemed more complicated than the bug fix code.
• I have also confirmed that the issues are not reproduced in flems above using fixed bundle.

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]

I am a little sorry for my frequent pull requests these days.😢
I did not plan for this pull request, but it seemed like an easy bug to fix...

I think these are bugs that almost no one cares about, so I won't hurry you to review or merge it.

[dead-claudia]

You still need to fix the diff algorithm (this will have negative perf impact) to compare is values for non-custom element vnodes.

• If the is attribute disappears, the element needs replaced so it's no longer a custom element.
• If the is attribute is newly added, the element needs replaced so it's now a custom element.
• If the is attribute changes, the backing custom element needs to change to be that new type.

In all three cases, it's simpler and more predictable to just remove and re-create.

[kfule]

@dead-claudia
Thanks for the comment. As you commented, this PR is going to have unpredictable behavior for users (even if it is straightforward javascript behavior). I will take some time to think about it some more.

I think I will close this PR for now. There is no hurry at all to fix the other bugs in this PR.

[dead-claudia]

@dead-claudia

Thanks for the comment. As you commented, this PR is going to have unpredictable behavior for users (even if it is straightforward javascript behavior). I will take some time to think about it some more.

I think I will close this PR for now. There is no hurry at all to fix the other bugs in this PR.

@kfule The current behavior isn't any more "predictable". What I suggested was really just a one line fix plus some more (pretty straightforward) tests. Sorry if this wasn't clear.

[kfule]

@dead-claudia
I have reopened this PR. I would like to request your review.

The following changes related to attr/style have been added to the previous PR. Both are small changes, I think.

• remove key === "is" from removeAttr() as well (mainly for code consistency with setAttr())
• remove cssText property access (cf. Bypass css text #2811)

I have not added any tests, but I would like to add tests if needed.

---

Also, I thought about your comment about "is" being "unpredictable".
When the value of "is" is set/changed/deleted, it would be better to delete the element and re-create it as you commented.
For instance, it would look fine to change the call updateElement() as follows:

default: {
	if ((old.attrs && old.attrs.is) == (vnode.attrs && vnode.attrs.is)) {
		updateElement(old, vnode, hooks, ns)
	} else {
		removeNode(parent, old)
		createElement(parent, vnode, hooks, ns, nextSibling)
	}
}

However, I personally would be more careful about adding code to this area because of the performance impact (about a several percent drop).
The "is" attribute has not yet been implemented in safari and does not seem to be planned to be implemented. I don't think I will be using “is” for a while yet.

[dead-claudia]

However, I personally would be more careful about adding code to this area because of the performance impact (about a several percent drop).

@kfule Try adding a bits field to the vnode that has the lowest bit set (var IS_CUSTOMIED_BUILTIN = 1 << 0) if it's a customized built-in. There's slight memory tradeoff (10 -> 11 fields, or 52 -> 56 bytes in V8), but it won't impact much else. Be sure to add it as the first field so it's most likely to reside on the same cache page as the tag.

A bit test from a field allocated inline in the object is very cheap. It's only a few bottlenecking instructions (4-6 at most) in the fast path plus an extra immediate generation + memory store (that won't bottleneck), so it should avoid significant perf deviations.

You'll also have to do a hasOwn.call(vnode.attrs, "is") check in m, which will cause some extra perf degradation, but doing it in m pretty much ensures it'll be fetching from cache, and this will save a lot of time. Plus, it's a pure existence check, so engines can skip some of the property value access work, and the use of hasOwn avoids the prototype walk (and attempt to set up inline caches on the result).

[kfule]

@dead-claudia
Thank you for your comment including great insight!
I think a 1-bit flag can not evaluate the change of is (e.g. "foo" -> "bar"). However, I found that adding an additional is field to the vnode itself (like vnode.key) and setting it in m(), as you suggested, keeps the performance almost the same as in v2.2.13.

So, I have added code and tests to this PR to recreate the node if the is is changed. I think “unpredictable” has been improved by this PR.

[kfule]

For instance, it would look fine to change the call updateElement() as follows:

Before "the call updateElement()", there was already a process that assumed an update to the same dom.
https://github.com/MithrilJS/mithril.js/blob/v2.2.13/render/render.js#L400-L402
https://github.com/MithrilJS/mithril.js/blob/v2.2.13/render/render.js#L405

Therefore, it is needed to evaluate is before those processes, and it is safer to evaluate it at the beginning as well as tag after all.

[dead-claudia]

@dead-claudia

Thank you for your comment including great insight!

I think a 1-bit flag can not evaluate the change of is (e.g. "foo" -> "bar"). However, I found that adding an additional is field to the vnode itself (like vnode.key) and setting it in m(), as you suggested, keeps the performance almost the same as in v2.2.13.

The idea wasn't to replace the field access, but to not pay the cost of checking it unless it's actually there.

Hope this helps.

---

A dedicated field is fine, though.

[dead-claudia]

Did you confirm this to be necessary while benchmarking?

What's the tradeoff here? I didn't see anything in the commit message clarifying why it's necessary.

If it's not actually boosting anything, please revert it. It was originally made to use for ... in for perf reasons, so I don't want that to change unless it needs to.

[kfule]

OK, I'll revert back to the previous code.
The purpose of this change was to reduce the amount of code rather than for performance purposes.
I should have detailed the purpose as it is not directly related to this PR.

[kfule]

@dead-claudia
Sorry, I misunderstood the use of flag you suggested... 😅
However, if there are no problems, I will leave the current dedicated field is as it is. Since the current code has a better look and feel.
I will revert isEmpty() and request the review again.

[kfule]

Sorry, I found vnode.attrs.is and changed it to use vnode.is.
If there are no additional comments, I would like to make this the last commit for this PR.

[dead-claudia]

Sorry, didn't do a detailed review the first time, my apologies.

This should be the last of my questions and requests.

[dead-claudia]

That's what the inner attrs[key] == null condition is for in the remove loop.

Is there a test case that failed with the old code? If not, I'd like to either see one, a benchmark result, or a bundle size reduction to justify the restructuring here.

[kfule]

That's what the inner attrs[key] == null condition is for in the remove loop.

Yes, that condition is the cause of the deletion issue.

An example of a test case that does not work well with the old mithril code is the flems described in the description at the top of this PR.

The performance does not seem to be affected by this code swap.
As for the bundle size, the bundle size is reduced by 1 byte gzipped due to the console.warn move (the code old && is removed).

[kfule]

I changed only this part and measured the bundle size.

original (current main branch)
Original size: 20,075 bytes gzipped (67,460 bytes uncompressed)
Compiled size: 8,960 bytes gzipped (24,405 bytes uncompressed)

original + swap (without console.warn move)
Original size: 20,082 bytes gzipped (67,460 bytes uncompressed)
Compiled size: 8,963 bytes gzipped (24,405 bytes uncompressed)

original + swap like this PR (with console.warn move)
Original size: 20,081 bytes gzipped (67,456 bytes uncompressed)
Compiled size: 8,959 bytes gzipped (24,402 bytes uncompressed)

[dead-claudia]

An example of a test case that does not work well with the old mithril code is the flems described in the description at the top of this PR.

Could you add the test case to this PR (if you don't have similar already)?

[kfule]

Would a manual test case (like in #3002) be acceptable?
I think the dom mock does not emulate the special case handling of such cases.

[kfule]

I added manual test similar to flems.

[kfule]

@dead-claudia
Thanks for the review comment!
I'll take a look at each of your comments.

[kfule]

I have benchmarked the variations related to this PR with npm run perf and the results are posted below.
(My laptop, used for the npm run perf in the other PR, seemed vulnerable to the room heater, so I used a mac mini (m2, node18) for this benchmark instead.)

results

results of `npm run perf`

main (f5fc394)

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	61108	61061	60595	60163	61104	61,108	60,163	60,920.00	100.00	99.38
rerender identical vnode	13002420	12506618	14961358	12537103	14542816	14,961,358	12,506,618	13,360,779.67	100.00	92.86
rerender same tree	290258	292898	294013	293141	294691	294,691	290,258	293,350.67	100.00	101.98
add large nested tree	26695	26660	26562	26485	26669	26,695	26,485	26,630.33	100.00	103.48
mutate styles/properties	591	600	608	597	592	608	591	596.33	100.00	99.94
repeated add/removal	11788	11691	11616	11688	11612	11,788	11,612	11,665.00	100.00	98.69

main + (old.attrs && old.attrs.is) == (vnode.attrs && vnode.attrs.is) in updateNode()

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	61175	60950	60981	60989	61067	61,175	60,950	61,012.33	100.15	99.53
rerender identical vnode	13920420	13989431	13744925	14093254	14491479	14,491,479	13,744,925	14,001,035.00	104.79	97.31
rerender same tree	273200	273269	271933	274370	273241	274,370	271,933	273,236.67	93.14	94.99
add large nested tree	24918	24756	24778	24771	24885	24,918	24,756	24,811.33	93.17	96.42
mutate styles/properties	586	581	589	579	588	589	579	585.00	98.10	98.04
repeated add/removal	11559	11793	11676	11626	11629	11,793	11,559	11,643.67	99.82	98.51

if (hasOwn.call(attrs, "is")) vnode.is = attrs.is in execSelector()

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	61,327	61,480	61,196	61,321	61,254	61,480	61,196	61,300.67	100.62	100.00
rerender identical vnode	15,004,056	14,477,669	14,549,853	14,137,579	13,864,380	15,004,056	13,864,380	14,388,367.00	107.69	100.00
rerender same tree	287,396	287,269	288,301	286,708	289,106	289,106	286,708	287,655.33	98.06	100.00
add large nested tree	25,761	25,832	25,809	25,291	25,631	25,832	25,291	25,733.67	96.63	100.00
mutate styles/properties	603	595	590	592	605	605	590	596.67	100.06	100.00
repeated add/removal	12,029	11,794	11,785	11,880	11,740	12,029	11,740	11,819.67	101.33	100.00

if (attrs.is != null) vnode.is = attrs.is in execSelector()

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	61,899	61,638	61,534	61,174	61,288	61,899	61,174	61,486.67	100.93	100.30
rerender identical vnode	14,144,867	13,120,051	14,076,482	13,604,859	14,118,253	14,144,867	13,120,051	13,933,198.00	104.28	96.84
rerender same tree	290,916	289,009	286,949	285,623	289,115	290,916	285,623	288,357.67	98.30	100.24
add large nested tree	25,920	25,946	25,820	25,601	25,639	25,946	25,601	25,793.00	96.86	100.23
mutate styles/properties	596	595	597	593	595	597	593	595.33	99.83	99.78
repeated add/removal	12,022	11,889	12,207	12,035	11,899	12,207	11,889	11,985.33	102.75	101.40

vnode.is = attrs.is in execSelector()

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	61,076	61,411	61,293	60,947	61,178	61,411	60,947	61,182.33	100.43	99.81
rerender identical vnode	13,528,635	14,029,221	13,940,756	13,147,634	12,280,696	14,029,221	12,280,696	13,539,008.33	101.33	94.10
rerender same tree	287,618	287,406	278,724	290,739	285,865	290,739	278,724	286,963.00	97.82	99.76
add large nested tree	25,543	25,723	25,774	25,628	25,667	25,774	25,543	25,672.67	96.40	99.76
mutate styles/properties	587	596	603	588	589	603	587	591.00	99.11	99.05
repeated add/removal	11,891	11,793	11,899	11,954	11,970	11,970	11,793	11,914.67	102.14	100.80

return Vnode("", attrs.key, attrs, children, undefined, undefined, attrs.is) in hyperscriptVnode()

	1	2	3	4	5	max	min	mid3-average	vs main	vs "hasOwn"
construct large vnode tree	63,397	62,940	62,880	63,038	62,527	63,397	62,527	62,952.67	103.34	102.69
rerender identical vnode	14,014,474	13,530,013	14,834,992	14,889,124	13,559,975	14,889,124	13,530,013	14,136,480.33	105.81	98.25
rerender same tree	283,727	285,482	286,921	286,764	288,033	288,033	283,727	286,389.00	97.63	99.56
add large nested tree	26,089	26,113	25,401	26,112	26,025	26,113	25,401	26,075.33	97.92	101.33
mutate styles/properties	580	597	593	596	603	603	580	595.33	99.83	99.78
repeated add/removal	11,744	12,016	11,899	11,609	11,567	12,016	11,567	11,750.67	100.73	99.42

thoughts

• Compared to the main branch, simply adding (old.attrs && old.attrs.is) == (vnode.attrs && vnode.attrs.is) to updateNode() results in a 6 to 7% drop in "rerender same tree" and "add large nested tree". The performance drop due to this PR or its variations is smaller than that (about 2 to 3% drop, which I can personally accept.).
• It may be within the margin of error, but if (attrs.is != null) vnode.is = attrs.is seems to be a good choice.
• return Vnode("", attrs.key, attrs, children, undefined, undefined, attrs.is) in hyperscriptVnode() seems to be as good as if (attrs.is != null) vnode.is = attrs.is or possibly better.

[kfule]

@dead-claudia
Thank you for your review comments. I have replied to all your comments.

I have also measured the benchmark and it seems to me that for if (hasOwn.call(attrs, "is")) vnode.is = attrs.is it would be better to change to either of the following.

• if (attrs.is != null) vnode.is = attrs.is in execSelector()
• return Vnode("", attrs.key, attrs, children, undefined, undefined, attrs.is) in hyperscriptVnode()

Also, there is a case where is is defined in the CSS selector, so it looks better to set it in execSelector().

[dead-claudia]

@kfule Try vnode.is = attrs.is in execSelector, without the condition. That'd be best for performance (1 optimized lookup + 1 store), and you could then drop the extra argument to Vnode. Doing the set there only would also ensure that it only works for elements and not as a magic positional key for other stuff (which, for design reasons, I'd rather not have - such magic properties should be usable without exception, so custom element vnodes can leverage them).

[dead-claudia]

Let me know how this impacts performance. Once this is addressed, I'm more than happy to merge.

[dead-claudia]

Edit: also add #2988 (comment) to the list of changes needed.

[kfule]

@dead-claudia
I have added a manual test code.

As I mentioned in my comment above, the benchmark results showed that if (attrs.is != null) vnode.is = attrs.is was slightly faster than vnode.is = attrs.is.
However, the difference is about 0.5%, so it may be within the margin of error.
I honestly don't care which code is used. Compared to the 7% drop, this is not a big problem.

[kfule]

To be sure, I ran the benchmark again, and it still seems to be faster with the if-condition (about 0.5%).

result of vnode.is = attrs.is (~0.5% slower?)

	1	2	3	4	5	max	min	mid3-average	vs main	vs "if (attrs.is != null)"
construct large vnode tree	61128	61463	61628	60831	60923	61,628	60,831	61,171.33	100.41	99.45
rerender identical vnode	12219128	14976356	14144199	14020772	13857496	14,976,356	12,219,128	14,007,489.00	104.84	98.78
rerender same tree	288328	289343	284116	283618	289411	289,411	283,618	287,262.33	97.92	99.51
add large nested tree	25748	25843	25750	25626	25674	25,843	25,626	25,724.00	96.60	99.96
mutate styles/properties	581	603	578	596	599	603	578	592.00	99.27	99.72
repeated add/removal	12656	11643	11700	11616	11636	12,656	11,616	11,659.67	99.95	100.13

result of if (attrs.is != null) vnode.is = attrs.is

	1	2	3	4	5	max	min	mid3-average	vs main	vs "if (attrs.is != null)"
construct large vnode tree	61500	61491	61837	61100	61547	61,837	61,100	61,512.67	100.97	100.00
rerender identical vnode	14527141	13936277	14911581	13119917	14079527	14,911,581	13,119,917	14,180,981.67	106.14	100.00
rerender same tree	288368	286976	290087	289016	288641	290,087	286,976	288,675.00	98.41	100.00
add large nested tree	25778	25658	25351	25808	25769	25,808	25,351	25,735.00	96.64	100.00
mutate styles/properties	594	604	579	590	597	604	579	593.67	99.55	100.00
repeated add/removal	11704	11886	11606	11624	11581	11,886	11,581	11,644.67	99.83	100.00

The if-condition case was still slightly faster, so the code is left as is for now.
As I commented above, I don't care which one you choose because the difference is not so big.