Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replacing msgpack-lite by msgpack due to data lost #184

Closed
wants to merge 1 commit into from
Closed

Replacing msgpack-lite by msgpack due to data lost #184

wants to merge 1 commit into from

Conversation

jeroenrinzema
Copy link

See issue #172

@darrachequesne
Copy link
Member

Were you able to reproduce the issue?

@jeroenrinzema
Copy link
Author

I was unfortunately not able to reproduce the bug in socket.io-redis but i was able to reproduce a part of the bug in msgpack-lite and msgpack-js. It looks like something goes wrong with the algorithm inside of the dependencies when encoding the message. I think that it is then anyhow a good idea to switch to a different dependency. msgpack passed the tests and didn't show the same strange behavior.

@darrachequesne
Copy link
Member

Without a proper way to reproduce, I'm afraid that we can't make sure it's really related to msgpack-lite...

Besides, it seems that msgpack does not compile under Windows: pgriess/node-msgpack#70

Regarding your example, may I ask what it is supposed to show?

screenshot from 2017-01-31 09 58 38

@jeroenrinzema
Copy link
Author

jeroenrinzema commented Jan 31, 2017

I see that something went wrong this is the right example. I will meanwhile keep on trying to reproduce the bug.

@darrachequesne
Copy link
Member

Thanks for the example.

It might be linked to #157. Are you still using the return_buffers option? Or a custom subEvent?

@valeriansaliou
Copy link

valeriansaliou commented Apr 27, 2017

Why don't we consider just dropping MessagePack? We've been running SocketIO-Redis @crisp-im over 1+ year, and as we are gaining more and more pressure on our SocketIO workers over time we've started reviewing what we could optimize.

It turned out that SocketIO-Redis use of MessagePack was causing ~20% of our SocketIO workers CPU load (notice: we do stream some large JSON payloads sometimes). Forking SocketIO-Redis and moving it to the Node native JSON.parse() / JSON.stringify() implementation fixed the issue.

Plus, looking at the Redis MONITOR command; it appears that MessagePack does not reduce the size of messages that much, as MessagePack-optimized binary blobs get string-encoded when wired in/out of Redis (Redis only accepts string contents). Thus, a lot of MessagePack compact-binary blobs get encoded/escaped as multiple-chars strings; which increase the overall size of the MessagePack payload that is streamed across SocketIO workers.

Overall, after switching from MessagePack to bare-bone JSON we've seen an overall CPU usage decrease of ~20%, and a Redis Pub/Sub I/O increase of ~10-20%. Which is why we'll stick to our JSON fork over the long run as usually high-pressure is set on the CPU at scale.

--

Anyway, if you want to stick to a MessagePack implementation, I'd recommend Notepack. We've been previously using it on other high-load services (to pass data between our internal Node workers using RabbitMQ) w/o that much impact on CPU (however we've also replaced it w/ JSON). If you perform some benchmark between Node MessagePack implementations, you'll notice Notepack is the fastest. We did not encounter any issue using it (encoding special extended UTF-8 chars; encoding/decoding large JSON, etc).

@darrachequesne
Copy link
Member

Closed by #218.

@darrachequesne darrachequesne added this to the 5.0.0 milestone May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants