search

mcollina / msgpack5

A msgpack v5 implementation for node.js, with extension points / msgpack.org[Node]
MIT License
492 stars 76 forks source link

Incomplete buffer #13

Closed shinichy closed 9 years ago

shinichy commented 9 years ago

Hi, thank you for pointing out the performance issue. I found that bl.duplicate was the cause so I refactored decode method not to use it. I introduced internal tryDecode(buf, offset) method to keep track of current offset as you suggested. If everything goes well, tryDecode returns {value: , bytesConsumed: }, so decode (buf) method actually calls bl.consume(bytesConsumed) at last step. Otherwise, bl is intact.

I ran your benchmark with my new code again and it's as fast as master now!

new code

time 2726 decode/s 36683.78576669112

master

time 2706 decode/s 36954.91500369549

mcollina commented 9 years ago

This is impressive. GREAT!

mcollina commented 9 years ago

I would like to remove the header thing in the stream support, but we would need a major version bump.

What do you think? With this fix the header in the streams is not worth it.

shinichy commented 9 years ago

I agree. The header is not worth it anymore.

mcollina commented 9 years ago

I am quite full this week, would you like to send a PR for that too?

shinichy commented 9 years ago

OK. I'll work on it this weekend.