-
Notifications
You must be signed in to change notification settings - Fork 547
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
Improve performance of clearAllBlockInfoAtChunk, don't get immutable map #3905
Conversation
Your Pull Request was automatically labelled as: "🧹 Chores" |
cc: @md5sha256 - not sure if you had any specific reason to use getRawStorage instead of accessing it directly. |
Slimefun preview buildA Slimefun preview build is available for testing! https://preview-builds.walshy.dev/download/Slimefun/3905/1b2eb20b
|
Kudos, SonarCloud Quality Gate passed! |
Timing courtesy of @Skizzles - https://spark.lucko.me/5ZX5HHHvHi
44s -> 11s We're now hitting Javaland code taking the rest of the majority: 22.03% (7.8s) from BlockStorage rewrite wen:tm: |
Either an oversight or I was erring on the side of caution. Either way, the performance benefit outweighs the perceived safety of operating on The only comment I have-ish, (and I've never done this, there was a reason why) but is it good practice/possible to just store (cache) the immutable map in the BlockStorage instance since it's basically just a wrapper? This might become obsolete in the future, however. |
probably but we'd really want to have partial updates done when needed to it (which kinda breaks the immutable part as it isn't fully) and afaik you can't do that with any Java-native implementations today. There's also just the issue of added memory :( |
Yeah, fair enough. In that case LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
We're currently getting our ImmutableMap clone in clearAllBlockInfoAtChunk, instead we can unsafely get the storage and save the overhead of duplicating the map (which for large servers is massive).
Right now it's taking basically all of the compute time:
Proposed changes
Unsafely get the raw storage instead of our safe ImmutableMap.
Related Issues (if applicable)
Resolves #3432
Checklist
Nonnull
andNullable
annotations to my methods to indicate their behaviour for null values