💡 [major] Prefer MSGPack over JSON #93

christianblais · 2024-08-15T18:48:19Z

Cachette cached values aren't meant to be human readable. This lib is meant to be fast. Therefore, I believe we can achieve a little bit more speed by avoiding JSON serialization, and instead use more performant algorithms, such as msgpack. With this PR, I'm replacing JSON with msgpacker. We would incur a small hit on writes, but gain interesting wins on reads.

5000 writes;

Platform info:
==============
   Darwin 23.3.0 arm64
   Node.JS: 18.20.4
   V8: 10.2.154.26-node.37
   CPU: Apple M2 × 8
   Memory: 16 GB

Suite: serialize
================

✔ msgpack       126,894 ops/sec
✔ json          131,671 ops/sec

   msgpack        -3.63%    (126,894 ops/sec)   (avg: 7μs)
   json (#)           0%    (131,671 ops/sec)   (avg: 7μs)

┌─────────┬────────────────────────────────────────────────────┐
│ msgpack │ ████████████████████████████████████████████████   │
├─────────┼────────────────────────────────────────────────────┤
│ json    │ ██████████████████████████████████████████████████ │
└─────────┴────────────────────────────────────────────────────┘

5000 reads;

Platform info:
==============
   Darwin 23.3.0 arm64
   Node.JS: 18.20.4
   V8: 10.2.154.26-node.37
   CPU: Apple M2 × 8
   Memory: 16 GB

Suite: parse
================

✔ msgpack        82,745 ops/sec
✔ json           59,111 ops/sec

   msgpack       +39.98%     (82,745 ops/sec)   (avg: 12μs)
   json (#)           0%     (59,111 ops/sec)   (avg: 16μs)

┌─────────┬────────────────────────────────────────────────────┐
│ msgpack │ ██████████████████████████████████████████████████ │
├─────────┼────────────────────────────────────────────────────┤
│ json    │ ████████████████████████████████████               │
└─────────┴────────────────────────────────────────────────────┘

Given cachette is all about optimizing reads, I believe this to be a good trade-off. Here, assuming a 5:1 read:write ratio, we see a 34% overall gain.

1000 reads, 5000 writes

Platform info:
==============
   Darwin 23.3.0 arm64
   Node.JS: 18.20.4
   V8: 10.2.154.26-node.37
   CPU: Apple M2 × 8
   Memory: 16 GB

Suite: serialize
================

✔ msgpack        24,490 ops/sec
✔ json           18,211 ops/sec

   msgpack       +34.48%     (24,490 ops/sec)   (avg: 40μs)
   json (#)           0%     (18,211 ops/sec)   (avg: 54μs)

┌─────────┬────────────────────────────────────────────────────┐
│ msgpack │ ██████████████████████████████████████████████████ │
├─────────┼────────────────────────────────────────────────────┤
│ json    │ █████████████████████████████████████              │
└─────────┴────────────────────────────────────────────────────┘

Note: Careful, as this PR is not revertable. Values set as buffer depends on the code present in this PR to be deserialized.

christianblais · 2024-08-15T18:49:29Z

src/lib/RedisCache.ts


 import { CachableValue, CacheInstance } from './CacheInstance';

+const MPACK = new Packr({ moreTypes: true });


By enabling moreTypes, we get "free" support for Set and Map.

erykwarren · 2024-08-15T20:23:19Z

src/lib/RedisCache.ts

@@ -212,6 +206,10 @@ export class RedisCache extends CacheInstance {
      return false;
    }

+    if (value instanceof Buffer) {


I don't think this check will work. This function is passed the returned value from a redisClient.get() which will be a string. Incidently, the typing of the function parameter is wrong here 😱 . It should be

public static deserializeValue(value: string | null)

or something like that.

Urgh! Right, I tested it with redis directly, but Node's redis lib seems to transform everything into strings. Alright, let me use the string prefix trick, and then I'll rerun the benchmark, to ensure we don't take too much of a performance hit.

test/RedisCache_test.ts

ronjouch

👍, just a couple nits & questions
Before merge (now), aside local cachette unit tests: did you already run one of our services with this (using npm link), and confirm (with redis-cli) that everything behaves well and looks good?
(If feels valuable after point 2. above) After merge, please test in a staging service with manual redis-cli inspection that everything behaves as expected

src/lib/RedisCache.ts

ronjouch · 2024-08-19T15:03:26Z

package.json

@@ -72,6 +72,7 @@
  },
  "dependencies": {
    "ioredis": "5.x",
+    "msgpackr": "1.x",


@christianblais looks sane and no-dependencies. However, usual introducing-new-depencency questions: how did you pick it? Were there alternatives, and if yes, what led you to think it's the best one?

MessagePack is a well defined standard, so the question was — "which implementation should I pick?". There aren't a ton of options to pick from, and this one stands out as the clear winner in terms of love (number/frequency of commits), popularity, and speed.

Now, I'm aware a lib might be feature-complete with no bugs, hence the lack of activity. But looking at the downloads per week, once more, msgpackr stood out as the clear winner. That was the end of my investigation.

Yip that's what I meant. Okay with your annotated screenshot!

test/RedisCache_test.ts

erykwarren · 2024-08-19T17:31:03Z

src/lib/RedisCache.ts

@@ -212,6 +206,10 @@ export class RedisCache extends CacheInstance {
      return false;
    }

+    if (value.startsWith(RedisCache.MSGP_PREFIX)) {
+      return MPACK.unpack(Buffer.from(value.substring(RedisCache.MSGP_PREFIX.length), 'binary'));
+    }


👍 Can you also fix the type of the function param on line 190?

Since it isn't related to this changeset per se, I did it in another PR.

christianblais · 2024-08-20T13:53:10Z

Locally tested with two of our main services with no apparent issue. So what do you folk say... ship it?

AdelBachene

w00t

Co-authored-by: Ronan Jouchet <[email protected]>

christianblais commented Aug 15, 2024

View reviewed changes

erykwarren reviewed Aug 15, 2024

View reviewed changes

test/RedisCache_test.ts Show resolved Hide resolved

christianblais changed the title ~~Prefer MSGPack over JSON~~ 💡 [patch] Prefer MSGPack over JSON Aug 19, 2024

christianblais force-pushed the msgpack-instead-of-json branch from 0f52aab to 81f0b37 Compare August 19, 2024 14:12

christianblais marked this pull request as ready for review August 19, 2024 14:20

christianblais changed the title ~~💡 [patch] Prefer MSGPack over JSON~~ 💡 [major] Prefer MSGPack over JSON Aug 19, 2024

ronjouch reviewed Aug 19, 2024

View reviewed changes

erykwarren reviewed Aug 19, 2024

View reviewed changes

christianblais force-pushed the msgpack-instead-of-json branch from 51cd22e to bd4ea75 Compare August 19, 2024 17:48

AdelBachene approved these changes Aug 20, 2024

View reviewed changes

christianb-unito and others added 4 commits August 21, 2024 15:08

Prefer MSGPack over JSON

1139cc7

Use string prefix to identify buffer values

bcc3a33

Update test/RedisCache_test.ts

28f0517

Co-authored-by: Ronan Jouchet <[email protected]>

Update src/lib/RedisCache.ts

8a5b5ec

Co-authored-by: Ronan Jouchet <[email protected]>

christianblais force-pushed the msgpack-instead-of-json branch from bd4ea75 to 8a5b5ec Compare August 21, 2024 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💡 [major] Prefer MSGPack over JSON #93

💡 [major] Prefer MSGPack over JSON #93

christianblais commented Aug 15, 2024 •

edited by ronjouch

Loading

christianblais Aug 15, 2024 •

edited

Loading

erykwarren Aug 15, 2024

christianblais Aug 19, 2024

ronjouch left a comment

ronjouch Aug 19, 2024

christianblais Aug 19, 2024 •

edited

Loading

ronjouch Aug 19, 2024

erykwarren Aug 19, 2024

christianblais Aug 19, 2024

christianblais commented Aug 20, 2024

AdelBachene left a comment


		import { CachableValue, CacheInstance } from './CacheInstance';

		const MPACK = new Packr({ moreTypes: true });

💡 [major] Prefer MSGPack over JSON #93

Are you sure you want to change the base?

💡 [major] Prefer MSGPack over JSON #93

Conversation

christianblais commented Aug 15, 2024 • edited by ronjouch Loading

christianblais Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

erykwarren Aug 15, 2024

Choose a reason for hiding this comment

christianblais Aug 19, 2024

Choose a reason for hiding this comment

ronjouch left a comment

Choose a reason for hiding this comment

ronjouch Aug 19, 2024

Choose a reason for hiding this comment

christianblais Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

ronjouch Aug 19, 2024

Choose a reason for hiding this comment

erykwarren Aug 19, 2024

Choose a reason for hiding this comment

christianblais Aug 19, 2024

Choose a reason for hiding this comment

christianblais commented Aug 20, 2024

AdelBachene left a comment

Choose a reason for hiding this comment

christianblais commented Aug 15, 2024 •

edited by ronjouch

Loading

christianblais Aug 15, 2024 •

edited

Loading

christianblais Aug 19, 2024 •

edited

Loading