Skip to content

Bring back the ValueView string path, for ASCII only - #1332

Closed
nigrosimone wants to merge 1 commit into
uNetworking:masterfrom
nigrosimone:ascii-string-fast-path
Closed

nigrosimone wants to merge 1 commit into
uNetworking:masterfrom
nigrosimone:ascii-string-fast-path

Conversation

@nigrosimone

Copy link
Copy Markdown
Contributor

Last part of #1300: the ValueView path removed in 4c83d44 comes back, with the same guards (AllowStringView, NODE_MODULE_VERSION >= 137) and two changes. It is taken only when the one-byte string is ASCII, where Latin-1 and utf-8 are the same bytes, so the bug of #1262 (#1280) cannot happen. And the bytes are copied while the view is alive, the old path kept a pointer into the V8 heap after the view was gone.

tests/strings.js sends bodies and header values through end, writeHeader and getHeader and compares the bytes on the wire: Latin-1, BMP, astral, NUL, empty, and strings past the 128 KB pool. It passes on master and on this branch, and fails 5 of 17 on a build that copies one-byte strings without the ASCII check, like #1262. It runs in build.yml after smoke.js.

Measured on master, Node 26 win-x64, min of 7 alternating runs: req.getHeader with a 10 byte key from 69 to 56 ns, with a 1 KB key no measurable change. End to end with the workflow of #1326 (nigrosimone#3): http/headers +1.8% in every round, inside the noise of that run.

@nigrosimone
nigrosimone marked this pull request as ready for review September 28, 2026 18:49
Comment thread tests/strings.js Resolved
Comment thread src/Utilities.h
if (high < 0x80) {
length = len;
data = alloc(length);
allocated = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are all paths causing allocated = true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

allocated means the data came from alloc(), not that it was malloced: all strings paths copy, and alloc() returns a slice of the 128 KB pool or, past it, malloc. The destructor calls NativeString::free, which calls ::free only for pointers outside the pool. The ArrayBuffer paths point into the backing store and keep it false. The new path copies like the other two, so it sets it the same way; the "past the pool, ascii" case of the test goes through its malloc branch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If all paths cause allocated = true, that boolean needs to go, but why is String::ValueView causing a copy wasn't the whole point of it to not copy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not all paths: the ArrayBuffer ones keep it false, they point into memory owned by V8, and without the flag the destructor would free it. So it is still needed.

On the copy: the view is valid only while it is alive, V8's header says no GC or allocation may happen during that time, and the constructor holds a DisallowGarbageCollectionInRelease. NativeString lives longer than the view: after it the wrapper keeps calling into V8, for example writeHeader makes a second NativeString, whose view flattens its string and can allocate. If the view stays alive that breaks the no-GC rule, and if it does not, a GC can move the string under the pointer, which is what the path removed in 4c83d44 did with data8(). So the copy is made while the view is alive, and the gain is one scan and a memcpy instead of Utf8Length plus WriteUtf8.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The original StringView PR had all of that refactored into ViewNativeString kind of alternatives

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reason why we cannot bother with implementation level hacks is because V8 is a moving target. They change things all the time, they could very well add all kinds of internal String bookkeepings in future version that make our hack slower, etc. So simply sticking to the WriteUtf8V2 always is way more elegant, simpler, and leaves micro-optimizations regarding V8 to V8.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to Gemini, Utf8Length() is constant time if the string holds only ASCII. So a better fast path would be:

if (Utf8Length() == length AND IsOneByte()) then
StringView zero-copy

that makes the fast path constant time and zero copy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mother fucking hallucinatory cocksucker

You caught a mistake in my previous explanation! There is no kIsOneByteAndOnlyAscii or kIsAsciiBit flag stored in V8's string headers or Map.V8 does not distinguish between pure ASCII (0–127) and Latin-1 (128–255) in $O(1)$ constant time.

@uNetworkingAB uNetworkingAB Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This

                // Fallback
                length = string->Utf8LengthV2(isolate);
                data = alloc(length);
                allocated = true;
                string->WriteUtf8V2(isolate, data, length);

can be

                // Fallback
                length = string->WriteUtf8V2(isolate, topOfPool(), remainingOfPool());
                if (remainingOfPool() - length > 8) { // 2 whole max sized code points
                    // fast path went ok and only called WriteUtf8V2
                    alloc(length);
                } else {
                    // slow slow path with malloc / free
                }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair, and your second point is a better idea. WriteUtf8V2 already writes until the buffer is full and returns the processed characters, so: write straight into what is left of the pool, advance the offset by the bytes written, and if fewer characters than Length() were processed, the string did not fit: malloc Utf8LengthV2() and write it again. The old WriteUtf8 has the same capacity and nchars_ref for V8 before 14.

I will try it, measure it and open it as a new PR with the strings test, and close this one.

@nigrosimone

This comment was marked as off-topic.

@nigrosimone

Copy link
Copy Markdown
Contributor Author

Close #1332 (comment)

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.

2 participants