kordlib/kord

Think about changing Snowflakes/Permissions/DiscordBitSets/UserFlags/etc to value classes

MrPowerGamerBR opened this issue · 4 comments

I was creating a entity cache server with Kord, and one of the things that I've noticed is the huge memory footprint.

(Using -XX:+UseStringDeduplication)

In these tests, I'm caching...

Users: 15031 users
Guilds: 47614 guilds
Channels: 1578752 channels
Guild Members: 47615 members (68290 total)
Guild Roles: 47614 guild roles (1237487 total)
Guild Emojis: 47614 guild emojis (414569 total)
Guild Voice States: 47615 guild voice states (4226 total)
No Snowflake value classes:
Used Memory: 1427MiB

Snowflake Value classes:
Used Memory: 1347MiB

Snowflake Value class + SnowflakeMap (SnowflakeMap = a class that wraps fastutil's Long2ObjectOpenHashMap)
Used Memory: 1215MiB

Change `Permissions` and `DiscordBitSet` to be `value` and immutable:
Used Memory: 966MiB

Before:
https://cdn.discordapp.com/attachments/587324906702766226/1037434893254279308/image.png

After:
https://cdn.discordapp.com/attachments/587324906702766226/1037434893690478752/image.png

(The LightweightSnowflake still has a big footprint because Channels can have a nullable channel, so it ends up being boxed)

Even though this would cause a binary incompatibility, I think it would be a good change to reduce the memory footprint used by Kord.

The DiscordBitSet would be hard to change to value because it is mutable, and value classes cannot be mutable. I think that it would be nice to have MutableDiscordBitSet (or DiscordBitSetBuilder?) instead of allowing the current DiscordBitSet to be mutable.

The Snowflake would be hard to change to value due to its ULong constructor, which is used to coerce the value.

Changing the UserFlags and Permissions to value classes can be done in a straightforward manner.

zTrap commented

Maybe add this in 1.0.0? These changes is hard breaking for api, but its ok if change major version too

I already brought this up for 0.9.0, so maybe that's worth it, as 0.9.0 will break binary compatibility anyw and this shouldn't break source compatibility I am in favor of this

/cc @HopeBaron @lukellmann

zTrap commented

@DRSchlaubi actually if u want to keep some checks (like value.coerceIn(validValues) in Snowflake) it'll breaks source compatability. These checks should be moved to the new fun snowflake(value: ULong), or it will clash with the real constructor

#886 was merged and the changes can be tested using experiments-inline-value-classes-SNAPSHOT