msgpack / msgpack-node

MessagePack implementation for Node.js
Other
311 stars 72 forks source link

Update for msgpack-1.1.0 and Bump nan for node v4 #26

Closed mog422 closed 9 years ago

godsflaw commented 9 years ago

It compiles and ./run_tests passes, but I'm a little concerned that we've lost performance packing one large object (or array of objects). I don't think this is your fault @mog422, but it would be nice to understand why.

To reproduce the benchmark failure, use this command:

$ ./run_tests test/benchmark
(node) sys is deprecated. Use util instead.

benchmark

msgpack.pack: 3754ms, JSON.stringify: 1348ms
ratio of msgpack.pack/JSON.stringify: 2.78486646884273
✖ benchmark - msgpack.pack faster than JSON.stringify with array of 1m objects

The TL;DR of current benchmarks is that if we do many small objects iteratively, we loose performance crossing the node/native boundary, an the JSON serializer/deserializer outperforms msgpack. However, when we send one large object into msgpack it always managed to outperform JSON, as the node implementation of the serializer/deserializer was written in JS, and the native code is C/C++.

Something has changed. Perhaps the JSON code for node is now native, or msgpack-1.1.0 got slower, or the addon libraries (v8) don't perform as well. I'm just speculating, but if you or anyone else has any thoughts it would be nice to know the reason why, and if there is anything we can do to fix performance.

For now, I am going to merge some other outstanding stuff and version bump where needed, and then come back to this and likely version bump a major version.

godsflaw commented 9 years ago

Blah, very sorry, I merged some of the backlog and made a huge conflict mess out of this. Just so you know, I would like to merge this (https://github.com/pgriess/node-msgpack/pull/62) before the node 4.0.0 and msgpack 1.1.0 bump as well, but I introduced conflicts to that PR too. I'm hoping the original author can resolve them, if not, we can take a shot at it.

mog422 commented 9 years ago

I resolved conflict.

mog422 commented 9 years ago

msgpack master branch with node v0.12.7

$ ./run_tests test/benchmark

benchmark

msgpack.pack: 2633ms, JSON.stringify: 1509ms
ratio of msgpack.pack/JSON.stringify: 1.7448641484426772
✖ benchmark - msgpack.pack faster than JSON.stringify with array of 1m objects

Assertion Message: msgpack.pack: 2633ms, JSON.stringify: 1509ms

msgpack-v1.1.0 branch with node v4.0.0

$ ./run_tests test/benchmark
(node) sys is deprecated. Use util instead.

benchmark

msgpack.pack: 3430ms, JSON.stringify: 910ms
ratio of msgpack.pack/JSON.stringify: 3.769230769230769
✖ benchmark - msgpack.pack faster than JSON.stringify with array of 1m objects

Assertion Message: msgpack.pack: 3430ms, JSON.stringify: 910ms
godsflaw commented 9 years ago

ahh, so something changed in v0.12.7, good find.

19h commented 9 years ago

Can we get some progress with this? We'd love to deploy an official release in production - for now we went with mog422/msgpack-node@msgpack-1.1.0 in order to get 4.0.0 going. :beers:

godsflaw commented 9 years ago

merged, version bumped to 1.0.0, and pushed to npm

19h commented 9 years ago

Thanks!