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

Update msgpack library to avoid UInt64 errors #105

Closed
wants to merge 1 commit into from

Conversation

didaxRedux
Copy link

Emitting messages with external processes trigger errors on msgpack decoding of UInt64.
Detailed description can be found at creationix/msgpack-js#16

The solution is very quick... change msgpack library which socket.io-redis is based on.

@darrachequesne
Copy link
Member

Besides, msgpack5 seems to be the recommended implementation for nodejs (http://msgpack.org/).

Could you squash it to one commit please?

@didaxRedux
Copy link
Author

It's embarrassing but I am a beginner on github!
How can I make one commit?

@darrachequesne
Copy link
Member

git rebase -i HEAD~n, n being the number of commits in your branch, so git rebase -i HEAD~2 in your case.

You should see:

pick xxxxxxx Update package.json
pick yyyyyyy Update index.js

which should be replaced with:

pick xxxxxxx Replace msgpack-js with msgpack5 (or something like that)
squash yyyyyyy Update index.js

Not tested, but that should do the trick 😄

Corrected dependencies: msgpack5 instead og msgpack-js

Update index.js

Corrected dependencies: msgpack5 instead og msgpack-js
@didaxRedux didaxRedux force-pushed the didaxRedux-patch-1 branch from 1fc467a to 11513e2 Compare May 9, 2016 12:41
@didaxRedux
Copy link
Author

didaxRedux commented May 9, 2016

Eureka!
Thank you very much

@darrachequesne
Copy link
Member

I still have a concern about perf. According to this https://github.com/kawanet/msgpack-lite#benchmarks, msgpack5 might be slower than other implementations.

(the number of different implementations for msgpack is just crazy...)

@didaxRedux
Copy link
Author

It's true. There are many implementations and each claims to be somehow more powerful.
What do you suggest?

@darrachequesne
Copy link
Member

@CarsonF
Copy link
Contributor

CarsonF commented Nov 29, 2016

Just ran msgpack-lite's benchmark on my MacBook.

Node v7.2.0

Sorted by op/s:

$ ./lib/benchmark.js 10
operation                                                 |   op   |   ms  |  op/s
--------------------------------------------------------- | -----: | ----: | -----:
buf = require("notepack").encode(obj);                    | 862700 | 10001 |  86261
buf = Buffer(JSON.stringify(obj));                        | 853700 | 10000 |  85370
buf = Buffer(require("msgpack.codec").msgpack.pack(obj)); | 708100 | 10000 |  70810
buf = require("msgpack-lite").encode(obj);                | 537300 | 10000 |  53730
buf = require("msgpack").pack(obj);                       | 397400 | 10000 |  39740
buf = require("msgpack-js").encode(obj);                  | 127300 | 10000 |  12730
buf = require("msgpack-js-v5").encode(obj);               | 126300 | 10006 |  12622
buf = require("msgpack5")().encode(obj);                  |  71700 | 10002 |   7168

obj = require("notepack").decode(buf);                    | 599300 | 10000 |  59930
obj = JSON.parse(buf);                                    | 588200 | 10000 |  58820
obj = require("msgpack").unpack(buf);                     | 552400 | 10000 |  55240
obj = require("msgpack.codec").msgpack.unpack(buf);       | 545100 | 10001 |  54504
obj = require("msgpack-js-v5").decode(buf);               | 439200 | 10002 |  43911
obj = require("msgpack-js").decode(buf);                  | 434600 | 10000 |  43460
obj = require("msgpack-lite").decode(buf);                | 400800 | 10002 |  40071
obj = require("msgpack5")().decode(buf);                  | 175500 | 10004 |  17542
obj = require("msgpack-unpack").decode(buf);              |  49400 | 10017 |   4931

Quite different than the results listed on their readme. notepack seems like the obvious choice, but it doesn't seem maintained and only has 20 commits. I would be in favor of msgpack-lite

@darrachequesne
Copy link
Member

I am young, it is true; but in souls nobly born valor does not depend upon age.

Using https://github.com/endel/msgpack-benchmark

sample-datatypes.json:

(encode) JSON x 166,633 ops/sec ±0.69% (101 runs sampled)
(encode) notepack x 68,807 ops/sec ±0.70% (101 runs sampled)
(encode) msgpack-javascript x 56,779 ops/sec ±1.18% (98 runs sampled)
(encode) msgpack-lite x 54,664 ops/sec ±1.37% (88 runs sampled)
(encode) msgpack x 40,741 ops/sec ±0.69% (99 runs sampled)
(encode) msgpack5 x 8,764 ops/sec ±2.36% (83 runs sampled)
(encode) msgpack-js-v5 x 14,499 ops/sec ±1.20% (94 runs sampled)
(decode) JSON x 87,976 ops/sec ±0.77% (97 runs sampled)
(decode) notepack x 50,225 ops/sec ±0.64% (98 runs sampled)
(decode) msgpack-lite x 44,857 ops/sec ±0.81% (96 runs sampled)
(decode) msgpack x 43,868 ops/sec ±0.56% (99 runs sampled)
(decode) msgpack-js-v5 x 31,444 ops/sec ±0.56% (101 runs sampled)
(decode) msgpack-javascript x 30,741 ops/sec ±0.83% (100 runs sampled)
(decode) msgpack5 x 12,319 ops/sec ±1.40% (90 runs sampled)

sample-small.json:

(encode) JSON x 973,970 ops/sec ±1.55% (99 runs sampled)
(encode) msgpack-javascript x 736,466 ops/sec ±0.59% (102 runs sampled)
(encode) notepack x 670,269 ops/sec ±0.78% (98 runs sampled)
(encode) msgpack-lite x 273,623 ops/sec ±2.37% (86 runs sampled)
(encode) msgpack x 203,104 ops/sec ±2.99% (91 runs sampled)
(encode) msgpack-js-v5 x 111,702 ops/sec ±1.02% (95 runs sampled)
(encode) msgpack5 x 47,426 ops/sec ±2.34% (75 runs sampled)
(decode) JSON x 981,695 ops/sec ±0.65% (99 runs sampled)
(decode) notepack x 668,146 ops/sec ±0.58% (97 runs sampled)
(decode) msgpack-lite x 523,267 ops/sec ±0.76% (100 runs sampled)
(decode) msgpack-javascript x 454,825 ops/sec ±1.14% (99 runs sampled)
(decode) msgpack-js-v5 x 338,607 ops/sec ±0.70% (100 runs sampled)
(decode) msgpack x 304,450 ops/sec ±0.80% (97 runs sampled)
(decode) msgpack5 x 80,013 ops/sec ±1.19% (94 runs sampled)

sample-medium.json:

(encode) JSON x 587,270 ops/sec ±0.84% (101 runs sampled)
(encode) msgpack-javascript x 344,674 ops/sec ±0.72% (97 runs sampled)
(encode) notepack x 232,403 ops/sec ±0.81% (98 runs sampled)
(encode) msgpack-lite x 132,499 ops/sec ±1.75% (93 runs sampled)
(encode) msgpack x 115,213 ops/sec ±2.17% (96 runs sampled)
(encode) msgpack-js-v5 x 35,421 ops/sec ±0.90% (96 runs sampled)
(encode) msgpack5 x 28,722 ops/sec ±2.46% (83 runs sampled)
(decode) JSON x 492,673 ops/sec ±0.45% (98 runs sampled)
(decode) msgpack x 157,595 ops/sec ±0.43% (101 runs sampled)
(decode) notepack x 156,346 ops/sec ±0.77% (96 runs sampled)
(decode) msgpack-lite x 146,432 ops/sec ±0.12% (101 runs sampled)
(decode) msgpack-javascript x 107,558 ops/sec ±0.45% (100 runs sampled)
(decode) msgpack-js-v5 x 83,377 ops/sec ±0.67% (101 runs sampled)
(decode) msgpack5 x 37,076 ops/sec ±1.59% (91 runs sampled)

sample-large.json:

(encode) JSON x 35,854 ops/sec ±0.99% (94 runs sampled)
(encode) notepack x 12,585 ops/sec ±1.71% (97 runs sampled)
(encode) msgpack-javascript x 9,796 ops/sec ±1.88% (94 runs sampled)
(encode) msgpack x 7,765 ops/sec ±0.97% (99 runs sampled)
(encode) msgpack-lite x 7,079 ops/sec ±5.16% (82 runs sampled)
(encode) msgpack-js-v5 x 1,684 ops/sec ±3.39% (92 runs sampled)
(encode) msgpack5 x 1,651 ops/sec ±3.15% (78 runs sampled)
(decode) JSON x 13,494 ops/sec ±0.92% (98 runs sampled)
(decode) msgpack x 9,285 ops/sec ±0.88% (98 runs sampled)
(decode) notepack x 7,361 ops/sec ±5.28% (90 runs sampled)
(decode) msgpack-lite x 7,169 ops/sec ±4.44% (92 runs sampled)
(decode) msgpack-javascript x 4,142 ops/sec ±1.07% (98 runs sampled)
(decode) msgpack-js-v5 x 3,859 ops/sec ±3.09% (87 runs sampled)
(decode) msgpack5 x 2,200 ops/sec ±3.87% (90 runs sampled)

Since it passes all tests, how about using notepack? Is there any need for custom codec?

@CarsonF
Copy link
Contributor

CarsonF commented Dec 1, 2016

notepack doesn't seem to work for me. Gives me:

readUInt32BE is not a function

Ironic lol.

I used the same buffer to test msgpack and msgpack-lite as well. msgpack-lite is the only one that decodes it.

@darrachequesne
Copy link
Member

@CarsonF how can I reproduce? Are you trying to decode a buffer encoded by another implementation?

@CarsonF
Copy link
Contributor

CarsonF commented Dec 2, 2016

Using a PHP implementation https://github.com/rybakit/msgpack.php

I could try to condense the code that creates the socket.io packet that's published to Redis into a snippet if it would help. But since it doesn't get past decoding on node's side I don't think it matters

@CarsonF
Copy link
Contributor

CarsonF commented Dec 2, 2016

If you don't want to try to figure out the php stuff here's this:

$packed = (new MessagePack\Packer())->pack(['foo' => 'bar']);
$packed = base64_encode($packed);
echo $packed; // gaNmb2+jYmFy

That should be able to be base64 decoded and then decoded from msgpack to give {foo: 'bar'}

@CarsonF
Copy link
Contributor

CarsonF commented Dec 7, 2016

@darrachequesne Any luck?

@darrachequesne
Copy link
Member

Closed by #156.

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