backport iojs v1.0.0 streams changes

[chrisdickinson]

This backports the various streams changes as of iojs v1.0.0, including tests.

R=@rvagg

[rvagg]

did you do this manually or did you use build/build.js in some way?

[chrisdickinson]

This was done semi-manually (with rewrite-js and bash). I totally missed the build.js file.

[chrisdickinson]

Should be fixed now. The first commit updates build.js, the second updates lib, and the third updates the test dir. These should probably be squashed before merging.

[sonewman]

I'm intrigued does this mean that the a writable no longer has an array of outgoing buffers?

[sonewman]

It is quite crazy actually as I had been pondering on the idea of whether a linked list would improve performance stream performance sometime before xmas.

[chrisdickinson]

@sonewman Yep, writable is now backed by a queue (based on a linked list) so that repeated small writes no longer thrash GC quite so badly. Prior to this, for most writable streams backed by async resources (re: "fs.WriteStream"), each fulfilled WriteReq would attempt to clearBuffer, make it one element into the array, and slice off the rest. This copied the array (increasing GC pressure) and generated a lot of large garbage arrays. The net result is that stream.Writable is much better suited to log streams piped to disk than it was prior.

[sonewman]

That's really awesome, good work dude 😃

[sonewman]

I'm wondering if we could do the same thing for the readable buffer.

I found it really interesting experimenting with linked lists, after a certain amount they were much quicker to iterate over than an array is. It must have something to do with the memory allocation and optimisation for smaller arrays (not that i know anything c++ or v8). My tests showed considerable difference 100,000 items, - which is an unlikely amount of buffers to have in a readable stream waiting to be consumed in process memory. But still, i will have to dig them out and put them in a gist somewhere.

If for some reason the stream was destroyed though, and the first item in the list was set to null, I assume that would GC the remaining items?

[chrisdickinson]

@sonewman I implemented a queue on the readable side, but it seems like it didn't move the needle at all in terms of performance. I'll dig up the code so you can take a look. Edit: here it is.

[sonewman]

I am having some difficulty applying the patch, but I am interested in this idea, so I am going to take a look tomorrow (as it is 23:23 in the UK).

Happy Friday 🎉

[sonewman]

@chrisdickinson I managed to apply your patch manually & I see what you mean. It gives pretty much the exact same bench on paused/readable led streams (obviously flowing on data streams don't hit the buffer anyway).

I was going to have fiddle and see if changing the prototype methods to plain functions would make any changes, in any case the speed is a matter of milliseconds (suggesting that this is incredibly well optimised either way).

The other interesting idea I had (which could be controversial) was whether there was any optimisation to be had by using a linked list in an EventEmitter. I will perhaps have a play with that at some point in the week.

I am happy to merge this, but It would be great to get some more eyes on it first @iojs/streams.

[sonewman]

Any objections to this? Because otherwise this should just go in since it's already in io.js

[isaacs]

Rubber-stamp LGTM. Everything matches my expectations of what's released in io.js today.

Moving forward, of course, changes should be made here first, and then merged into io.js. Anything already in io.js should certainly be backported here.

[sonewman]

Yeah I agree totally. OK, in it goes...