Skip to content

speedup ByteString and well-formed USVString #54

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.

Sign up for GitHub

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

Open
ChALkeR wants to merge 1 commit into
base: main
Choose a base branch
from
Open

speedup ByteString and well-formed USVString #54

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,18 +252,23 @@ exports.DOMString = (value, options = {}) => {

exports.ByteString = (value, options = {}) => {
const x = exports.DOMString(value, options);
let c;
for (let i = 0; (c = x.codePointAt(i)) !== undefined; ++i) {
if (c > 255) {
throw makeException(TypeError, "is not a valid ByteString", options);
}

// Unicode regex is identical here but is ~5x slower on 16-bit strings
// eslint-disable-next-line require-unicode-regexp
Copy link
Member

@domenic domenic Dec 23, 2025

Choose a reason for hiding this comment

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

Please don't disable lints; fix the code to pass them instead.

Copy link
Author

@ChALkeR ChALkeR Dec 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why? There should be no need for /u for this specific usecase, and it's 5x slower for wide strings.
I understand why /u is preferred in general as a rule, but not for testing ascii or single-byte

Copy link
Author

@ChALkeR ChALkeR Dec 23, 2025

Choose a reason for hiding this comment

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

I added a comment

Copy link
Author

@ChALkeR ChALkeR Dec 23, 2025

Choose a reason for hiding this comment

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

This regex:

ByteString, len = 100 x 38,977,623 ops/sec @ 25ns/op (0ns..1206μs)
ByteString, len = 10,000 x 39,783,208 ops/sec @ 25ns/op (0ns..55μs)
ByteString, len = 1,000,000 x 39,833,400 ops/sec @ 25ns/op (0ns..64μs)
ByteString, len = 100,000,000 x 40,453,673 ops/sec @ 24ns/op (0ns..351μs)
ByteString, len = 500,000,000 x 40,991,163 ops/sec @ 24ns/op (0ns..4ms)

ByteString, wide, len = 100 x 11,596,562 ops/sec @ 86ns/op (0ns..970μs)
ByteString, wide, len = 10,000 x 198,745 ops/sec @ 5μs/op (4μs..95μs)
ByteString, wide, len = 1,000,000 x 2,007 ops/sec @ 498μs/op (493μs..668μs)
ByteString, wide, len = 100,000,000 x 20 ops/sec @ 50ms/op (49ms..53ms)
ByteString, wide, len = 500,000,000 x 4.0 ops/sec @ 248ms/op (247ms..250ms)

Unicode mode regex:

ByteString, len = 100 x 41,036,785 ops/sec @ 24ns/op (0ns..1010μs)
ByteString, len = 10,000 x 39,797,708 ops/sec @ 25ns/op (0ns..113μs)
ByteString, len = 1,000,000 x 39,954,124 ops/sec @ 25ns/op (0ns..46μs)
ByteString, len = 100,000,000 x 41,130,007 ops/sec @ 24ns/op (0ns..513μs)
ByteString, len = 500,000,000 x 41,435,116 ops/sec @ 24ns/op (0ns..603μs)

ByteString, wide, len = 100 x 2,812,106 ops/sec @ 355ns/op (250ns..57μs)
ByteString, wide, len = 10,000 x 31,405 ops/sec @ 31μs/op (29μs..113μs)
ByteString, wide, len = 1,000,000 x 314 ops/sec @ 3ms/op (2ms..3ms)
ByteString, wide, len = 100,000,000 x 3.1 ops/sec @ 317ms/op (317ms..318ms)
ByteString, wide, len = 500,000,000 x 0.63 ops/sec @ 1599ms/op

That is a huge perf impact without any behavior differences here

Copy link
Member

@domenic domenic Dec 23, 2025

Choose a reason for hiding this comment

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

It's unfortunate that programmers have to care about the performance difference here and cannot just enforce lint rules generally. I guess a comment is the best we can do, maybe with a link to a V8 bug.

if (!/[^\x00-\xff]/.test(x)) {
return x;
}

return x;
throw makeException(TypeError, "is not a valid ByteString", options);
};

exports.USVString = (value, options = {}) => {
Copy link
Member

@domenic domenic Dec 23, 2025

Choose a reason for hiding this comment

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

Can't we just replace this whole function with toWellFormed()?

Copy link
Author

@ChALkeR ChALkeR Dec 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Done! (When present)

Copy link
Member

@domenic domenic Dec 23, 2025

Choose a reason for hiding this comment

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

Let's just replace it wholesale, no need for existence testing. Node.js v20 is the minimum version requirement.

const S = exports.DOMString(value, options);

if (S.toWellFormed) {
return S.toWellFormed();
}

const n = S.length;
const U = [];
for (let i = 0; i < n; ++i) {
Expand Down