Write directly to the Stream

[mcollina]

I have done some in-deep performance analysis on this library and MQTT.js, and I was able to squeeze roughly a 5x throughput improvement over a TCP socket, by running slower code that allocates less memory.

On this library, using the old generate:

Total time 4958
Total packets 1000000
Packet/s 201694.2315449778

On this library, using writeToStream:

Total time 3062
Total packets 1000000
Packet/s 326583.9320705421

Which is roughly 60% faster than the old generate().

On my system, I was able to ramp up the QoS 0 throughput on MQTT.js from 40k msg/s to 200k msg/s on node v4.2.0 using the stream.cork() and stream.uncork() technique (PR to follow).

This pull requests introduces that logic and removes the full packet allocation and subsequent buffer.write*() and buffer.copy() calls. Most of the contents of a packet are only allocated once and cached forever. Even the lengthy algorithm for calculating a length is memoized. On a new PUBLISH packet, the only things that are allocated are the topic and the payload.

All this has a downside: the old generate() method is 50% slower.

I propose to bump the MAJOR for this, as it changes a lot, and it might broke existing code. Not sure about MQTT.js, but we'll discuss it there.

I would like if some of you review this code: @adamvr @pmorjan @psorowka @sandro-k @xadh00m @mdickens @kokeksibir

Thanks to @trevnorris for the discussion at nodeconf.eu, and to @thlorenz for cpuprofilify which validated @trevnorris intuition.

[sandro-k]

👍 but I will need some time to look at this..

[mcollina]

@sandro-k there is some heavy caching of binary representation of numbers and packet length that might cause problems in the Browser. If you can check it, it would be awesome. I'm planning to get this merged & released by the end of the week, so we can start upgrading MQTT.js after (I already have the code ready). Anyway, there will be time for later fixes if you can't review.

[sandro-k]

just make a RC and we will see from there

[kokeksibir]

I guess you have left cork/uncork calls out of writeStream because of backwards compatibility, right? Even though I'd like to have a short-hand alternative that implicitly does cork/uncork maybe something like,

function alternative_generate(packet, stream) {
    stream.cork();
    generate(packet, stream);
    process.nextTick(uncork, stream);
}

[mcollina]

Exactly, the main reason I've left it out is for backward compatibility, and because I would like the other modules using this to do their cork/uncork as they see fit. Possibly the process.nextTick has some disadvantages that I am unaware off.

[trevnorris]

Commenting w/ partial context, but using nextTick() is how we uncork() in the 'http' module.

[mcollina]

@trevnorris thanks so much for providing context here! Would you say that using nextTick() is the best way for using uncork()? If so, I think we can just put this in and let the users of this module forget about it.

I would also protect it for backward compatibility:

    if (stream.cork) {
     stream.cork();
     process.nextTick(uncork, stream);
    }
    generate(packet, stream);

[trevnorris]

It is if you're trying to automate it for your users.

[kokeksibir]

👍

[mcollina]

Thanks @trevnorris! I added this to the PR then!

[trevnorris]

Also note that you can't collide calls. Meaning, if a user called cork/uncork inside your own it won't cause any problems with yours.

[kokeksibir]

Nice to hear 5x performance improvement, it does not happen all the time :) I just reviewed, it's OK for me, I dropped a small comment, though it is just a nice-to-have suggestion and may be unnecessary at all.

[mcollina]

Great! I'll get this merged and I'll get a release candidate out of the door, so we can work on MQTT.js.

[mcollina]

I'll release this as 4.0.0 in the next couple of days. If we need browser-specific fixes, we can always release 4.1.

[mcollina]

Released as 4.0.0. We'll bump the MINOR if we need to change things for the browser.