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

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

Open
MrPowerGamerBR opened this issue Nov 2, 2022 · 4 comments · Fixed by #886

Comments

@MrPowerGamerBR
Copy link
Contributor

MrPowerGamerBR commented Nov 2, 2022

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
Copy link

zTrap commented Apr 2, 2023

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

@DRSchlaubi
Copy link
Member

DRSchlaubi commented Apr 2, 2023

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
Copy link

zTrap commented Apr 2, 2023

@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

@lukellmann
Copy link
Member

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

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 a pull request may close this issue.

4 participants