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
(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.
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
@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