Repository navigation
Bring back the ValueView string path, for ASCII only - #1332
nigrosimone wants to merge 1 commit into
Conversation
| if (high < 0x80) { | ||
| length = len; | ||
| data = alloc(length); | ||
| allocated = true; |
There was a problem hiding this comment.
Why are all paths causing allocated = true
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The original StringView PR had all of that refactored into ViewNativeString kind of alternatives
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Close #1332 (comment) |
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.jssends bodies and header values throughend,writeHeaderandgetHeaderand 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.getHeaderwith 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.