dev: merge HTTP chunks during chunked encoding #47575
Conversation
|
Review requested:
|
nodejs-github-bot
added
http
mertcanaltin
changed the title
| @@ -931,7 +931,7 @@ function write_(msg, chunk, encoding, callback, fromEnd) { | |||
| let ret; | |||
| if (msg.chunkedEncoding && chunk.length !== 0) { | |||
| len ??= typeof chunk === 'string' ? Buffer.byteLength(chunk, encoding) : chunk.byteLength; | |||
| msg._send(NumberPrototypeToString(len, 16), 'latin1', null); | |||
| msg._send(NumberPrototypeToString(len, 16), 'ascii', null); | |||
There was a problem hiding this comment.
I think we shouldn't change this as this will incur unnecessary overhead as node has to parse the string to ensure it's valid ASCII (and we know it will already be valid because we're using the built-in Number.prototype.toString()). latin1 doesn't have such overhead since it's a single byte encoding.
There was a problem hiding this comment.
I'm confused. ASCI is also single byte
There was a problem hiding this comment.
Though I do see now that the change is probably unnecessary.
There was a problem hiding this comment.
We are a bit inconsistent when we use ascii and when we use latin1.
There was a problem hiding this comment.
Where/Why is there an overhead?
With 'ascii' node checks for every byte it's within the range 0-127 inclusive. With 'latin1' there's no such check.
O(n) vs. O(0), basically. :-)
|
The name of the PR and commit is misleading |
There was a problem hiding this comment.
Seems latin1 is to be preferred. https://github.com/nodejs/node/pull/47575/files#r1167745549
|
thank you for answers |
Use ASCII instead of Latin1 as encoding. ASCII is a subset of Latin1 and provides better compatibility across different systems and platforms @mcollina @ronag #57