dev: merge HTTP chunks during chunked encoding#47575
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Apr 15, 2023
Review requested:
|
nodejs-github-bot added http 
mertcanaltin changed the title 
| if(msg.chunkedEncoding&&chunk.length!==0){ | ||
| len??=typeofchunk==='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. :-)
ronag commented Apr 16, 2023
The name of the PR and commit is misleading |
ronag left a comment
ronag left a comment There was a problem hiding this comment.
Seems latin1 is to be preferred. https://github.com/nodejs/node/pull/47575/files#r1167745549
mertcanaltin commented Apr 16, 2023
thank you for answers |
5 participants
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