Skip to content

Script: len,is_empty and match for DOMString#39866

Merged
jdm merged 4 commits intoservo:mainfrom
Narfinger:lazydomstring-improvements
Oct 23, 2025
Merged

Script: len,is_empty and match for DOMString#39866
jdm merged 4 commits intoservo:mainfrom
Narfinger:lazydomstring-improvements

Conversation

@Narfinger
Copy link
Copy Markdown
Contributor

This replaces the implementation of is_empty and len with more efficient representation without conversion
and allocation based on the underlying bytes.
For this we use a new view, EncodedBytesView.
Additionally, we implement a new macro match_domstring_ascii! which allows simple match clauses
matching ascii strings with DOMStrings without conversion/allocation.
The macro will panic in debug builds if the strings are non-ascii but will not match all DOMStrings correctly.
We replaced the usage of DOMString::str() in many places with this macro.

Testing: len and is_empty were already covered by tests. match_domstring_ascii! has more unit tests added with this PR.

@Narfinger Narfinger requested a review from gterzian as a code owner October 14, 2025 12:40
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 14, 2025
@Narfinger Narfinger force-pushed the lazydomstring-improvements branch 2 times, most recently from 2549527 to bf76566 Compare October 15, 2025 14:26
@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 17, 2025

I've skimmed this PR and it's promising! I'm not convinced by the match macro yet; have you checked what the impact is on code size? I hope to finish reviewing these changes this weekend.

@jdm jdm added the S-awaiting-answer Someone asked a question that requires an answer. label Oct 17, 2025
@servo-highfive servo-highfive added the S-needs-rebase There are merge conflict errors. label Oct 18, 2025
We replace is_empty, len with more efficient methods based on the
underlying bytes.
Additionally, we introduce `match_domstring_ascii!` macro to efficiently
match &str in ascii with the domstring without having to convert.

Signed-off-by: Narfinger <[email protected]>
@Narfinger Narfinger force-pushed the lazydomstring-improvements branch from bf76566 to f9c13b5 Compare October 20, 2025 11:31
@servo-highfive servo-highfive removed the S-needs-rebase There are merge conflict errors. label Oct 20, 2025
@Narfinger Narfinger force-pushed the lazydomstring-improvements branch from f9c13b5 to eb87024 Compare October 20, 2025 11:34
@Narfinger
Copy link
Copy Markdown
Contributor Author

Narfinger commented Oct 20, 2025

Production build size increases from 113_970_968 to 113_975_664 and increase in 4696 bytes.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-awaiting-answer Someone asked a question that requires an answer. labels Oct 22, 2025
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 23, 2025
@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 23, 2025
@jdm jdm enabled auto-merge October 23, 2025 13:31
@jdm jdm added this pull request to the merge queue Oct 23, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 23, 2025
Merged via the queue into servo:main with commit 18cac42 Oct 23, 2025
33 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 23, 2025
@Narfinger Narfinger deleted the lazydomstring-improvements branch October 23, 2025 15:36
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.

4 participants