feat: @discordjs/redis-collection

[imranbarbhuiya]

trafficstars

Please describe the changes this PR makes and why it should be merged: redis collection Status and versioning classification:

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
discord-js	⬜️ Ignored (Inspect)		Sep 28, 2022 at 7:03AM (UTC)

[codecov[bot]]

Codecov Report

Merging #8556 (7b5974c) into main (3c231ae) will decrease coverage by 6.59%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8556      +/-   ##
==========================================
- Coverage   92.08%   85.48%   -6.60%     
==========================================
  Files          10       85      +75     
  Lines        2098     7987    +5889     
  Branches      240      959     +719     
==========================================
+ Hits         1932     6828    +4896     
- Misses        163     1117     +954     
- Partials        3       42      +39     

Flag	Coverage Δ
builders	100.00% <ø> (?)
collection	∅ <ø> (?)
proxy	80.46% <ø> (?)
redis-collection	100.00% <100.00%> (?)
rest	92.08% <ø> (ø)
utilities	100.00% <ø> (?)
voice	63.83% <ø> (?)
ws	60.47% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/redis-collection/src/utils.ts	100.00% <100.00%> (ø)
packages/ws/src/ws/WebSocketShard.ts	23.18% <0.00%> (ø)
packages/voice/src/audio/AudioPlayer.ts	80.95% <0.00%> (ø)
packages/builders/src/messages/embed/Assertions.ts	100.00% <0.00%> (ø)
packages/builders/src/components/Components.ts	100.00% <0.00%> (ø)
...ionCommandOptionWithChoicesAndAutocompleteMixin.ts	100.00% <0.00%> (ø)
packages/voice/src/networking/VoiceWebSocket.ts	0.00% <0.00%> (ø)
.../src/strategies/sharding/WorkerShardingStrategy.ts	95.87% <0.00%> (ø)
packages/builders/src/util/jsonEncodable.ts	100.00% <0.00%> (ø)
packages/builders/src/components/ActionRow.ts	100.00% <0.00%> (ø)
... and 65 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[DraftProducts]

This is a really nice addition 🚀 This coordinates with @kyranet's efforts to reduce discord.js cache usage. However, is there any particular reason for using ioredis rather than node-redis ?

[kyranet]

I don't think, to be honest, that the Redis collection will actually save a significant amount of memory.

And the reason why ioredis over node-redis, is because there's literally no reason to use the latter. The former comes with massive performance improvements along several other enhancements.

[DraftProducts]

If we could save payload of classes in redis, and rebuild on use, we could use more CPU and less memory ? But overwise Redis cache memory (built with C lang) management should be better when Node cache ? 🤔

[kyranet]

Problem is that the data is stored in KV pairs, whereas V8's objects store data in value-only arrays alongside a pointer to a Shape.

While binary data storage can be compact, KV storage can make it use more memory than V8. If you want to do something even more efficient, you'll need a custom aligned value-only binary RW, which can be application specific. In this case, yes, there might be a benefit in memory usage if we store data in a layout similar to C. I have written said RW for an application of mine (and later @didinele tried it on her project), but I have yet to measure the memory usage of discord.js vs value storage.

The bar for saving memory is very high, and such we can't have savings unless we do some complicated low-level code.

[sebasptsch]

This looks really helpful, I am wondering whether the cache-manager package was being considered to make this a little more versatile?

[KagChi]

Redis cluster support?

[imranbarbhuiya]

Since some people have an interest in this pr, so please add a reason before closing.

[iCrawl]

Because this is not actually what anyone wants. As much as you think they do.

1. This is a performance nightmare. All the serializing and deserializing happening here. If this were in the main lib to replace in-memory caching, you might as well run on an old Pentium 4 with a 1MB/s connection.
2. This does not utilize the JSON Redis module, which it honestly should because see point 1.
3. There are efficient solutions out there for how to store your data properly in your database and this should not be used as a "storing" layer for random things that should be saved properly. It just leads to a horrible "lead-by-example", because let's be real: Everyone would use this instead of storing stuff in a proper way (Whether that be actual K/V in Redis or SQL)

Edit: I forgot to mention one thing:

4. This could quickly derail into: https://www.npmjs.com/package/enmap. Which was/is anything but great. Collections or Maps should not be abused in this kind of way.