I've added a benchmark to check performance and it shows that this is as performant as the current implementation. However, the benchmark could definitely use a review as it's very easy to do benchmarks wrong and I have no particular expertise. @nodejs/benchmarking

@trevnorris @mscdex

I did some benchmarking outside of node recently that was related to this and IIRC concatenating strings was faster (rather than creating the string at the end)? This would need some double checking though.

By the way, the discussion in #1902 is probably relevant. The PR changed over time and some of the commits seem to be lost, but check out the comments by @piscisaureus.

@cjihrig I personally don't see a problem with going over by at most 3 bytes if the stream ends on a partial character, but you could always peek into the StringDecoder state to see how many bytes are buffered at the end of the stream to account for those bytes.

@mscdex While I'm not sure whether we can account for this, there are cases where the buffered character is larger. Here's an exaggerated case:

const s = 'ก็็็็็็็็็็็็็็็็็็็็';
console.log(s.length);  // 21
console.log(Buffer.from(s));  // 63

@trevnorris and Buffer.byteLength(s, 'utf8') === 63, so I still don't see what the problem is with the presented alternative solutions (mine or yours).

If you pass say Buffer.from(s).slice(0, 3) to a string decoder, a single character will be returned. Calling Buffer.byteLength() on that character will return 3 bytes. Concatenating the output from the string deocder still results in the same s value.

@mscdex Sorry, didn't convey my point. It's that while the character code may be complete it's also possible that the rendered character is incomplete because of missing diacritical marks. But also that this isn't a case we're able to handle.

For example:

Buffer('Á');  // <Buffer c3 81>
Buffer('Á');  // <Buffer 41 cc 81>

Appear to be the same character, but the first is actually '\u00c1' and the other is 'A\u0301'. So the rendered output on the other end is incomplete.

@trevnorris I still don't quite follow. Are you referring to a situation where Buffer.from([0x41,0xcc]) is passed to the string decoder but the needed 0x81 is never received from the actual stderr stream? If so, there is nothing that node or anyone else can ever do about that, but what matters is that we can count the bytes received. So in that particular case we'd see the 'A' emitted by the decoder which would count for one byte and at stderr end we can stderrLen += child.stderr._readableState.decoder.charReceived to account for the received 0xcc byte.

Now that I think about it, maybe yet another (better) solution is to just manually instantiate a string decoder and pass that the data chunks, that way we avoid having to peer into _readableState and the string decoder's state variables?

maybe yet another (better) solution is to just manually instantiate
a string decoder and pass that the data chunks, that way we avoid
having to peer into _readableState and the string decoder's state variables?

That's where I was leading with #6764 (comment)

@jasnell Ah ok. The only difference performance-wise with that last proposed solution is that it may be still be possible for streams to internally concatenate buffered Buffers (with Buffer.concat()) vs concatenating plain strings (when using .setEncoding()) before data is emitted? I'm not sure what that cost difference is.

Yeah, not sure either. Would need to benchmark it to be certain
On May 16, 2016 6:10 PM, "Brian White" notifications@github.com wrote:

@jasnell https://github.com/jasnell Ah ok. The only difference
performance-wise with that last proposed solution is that it may be still
be possible for streams to internally concatenate buffered Buffers (with
Buffer.concat()) vs concatenating plain strings (when using .setEncoding())
before data is emitted? I'm not sure what that cost difference is.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6764 (comment)

@mscdex This isn't about the string decoder. It's about the (very unlikely) possibility of data coming in, then say be logged. Where it would basically be:

console.log('A');
console.log('\u0301');

The rendered output would be wrong, even though each character is technically correct. I'm not saying we should consider supporting this properly. Just wanted to make the point that there are cases where even having the full utf8 characters won't render properly.

If someone was truly worried about that case they could buffer and wait for a new line to be received before calling console.log(). Not all that worried about that possibility

@trevnorris That's a separate issue though and node can't do anything about that anyway. Being able to count the bytes is the issue here, but even that separate issue isn't an issue for the majority of people that use exec() with a callback since the entire (buffered) string would be available for each stream (so it's not possible for users to console.log() multiple times unless they explicitly slice the string or also add their own stream event handlers, but that is a user problem at those points).

@mscdex sure thing. there was another comment that prompted me to make mention of this, and clarify that node doesn't have any intention of handling it.

I've changed the implementation to only run toString() on the buffer in the exit handler. My interpretation of the discussion above is that this still doesn't account for some unlikely edge cases, but that we probably can't do much about those anyway. PTAL.

@Trott You'd need to do it in the close event since data can still be received after exit. Nevermind, I see that exithandler is actually used for the close event.

CI seems to be experiencing less heartburn now than 2 days ago. Let's try again. CI: https://ci.nodejs.org/job/node-test-pull-request/2754/

OK... CI looks good... https://ci.nodejs.org/job/node-test-pull-request/2754/

Benchmarks are alarmingly on par:

$ ./node-pr-6764 benchmark/child_process/child-process-exec-stdout.js 
child_process/child-process-exec-stdout.js len=64 dur=5: 40871.77387
child_process/child-process-exec-stdout.js len=256 dur=5: 40870.66593
child_process/child-process-exec-stdout.js len=1024 dur=5: 40905.69407
child_process/child-process-exec-stdout.js len=4096 dur=5: 40880.96963
child_process/child-process-exec-stdout.js len=32768 dur=5: 40856.23579
$ node-6.2.0 benchmark/child_process/child-process-exec-stdout.js 
child_process/child-process-exec-stdout.js len=64 dur=5: 40872.87520
child_process/child-process-exec-stdout.js len=256 dur=5: 40868.80411
child_process/child-process-exec-stdout.js len=1024 dur=5: 40878.62745
child_process/child-process-exec-stdout.js len=4096 dur=5: 40876.45268
child_process/child-process-exec-stdout.js len=32768 dur=5: 40866.85730
$

Do we feel good about this as a solution to the problem? If so, can I get an LGTM or two? If not, what are the deficiencies?

CI is green, LGTM.

Landed in c9a5990

@Trott lts?

@thealphanerd If it lands cleanly, yes.