-
-
Notifications
You must be signed in to change notification settings - Fork 930
Make properties into records #2871
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
Conversation
worldedit-core/src/main/java/com/sk89q/worldedit/command/util/SuggestionHelper.java
Outdated
Show resolved
Hide resolved
|
Note that this makes subsequent changes to any of the fields impossible (e.g., using an |
|
Perhaps it would be worth doing that optimization now then, but considering we have to maintain the Stating this has no performance benefit needs proof: records are known to enable extra final field optimizations due to being more strictly initialized (you could test the difference #2477 made, we found it to be about 10% faster) and thus tend to increase performance. |
|
These classes need to be as absolutely thin/direct as possible, to avoid memory overhead issues on modded platforms, so this doesn't actually change anything about feasibility of altering underlying fields. We've intended to move these towards some form of set (likely sequenced) in WorldEdit 8. This is also step one of other changes, such as making the base Property interface sealed. Plus, in even rough simple testing there was found to be a measurable performance improvement, one that likely becomes more measurable in more complex (especially heavily modded) situations. |
A performance benefit requires the object itself to be constant at a given field access. That means that usages like https://github.com/EngineHub/WorldEdit/blob/version/7.3.x/worldedit-core/src/main/java/com/sk89q/worldedit/function/block/SnowSimulator.java#L38-L39 don't actually benefit from it at all. I don't see many usages that would benefit from the existing optimizations. Additionally, the current usage also includes Maps (in BlockState), but records are known for slow hashCode/equals implementations when non-primitive fields are involved (https://bugs.openjdk.org/browse/JDK-8366424, only fixed in Java 26). While |
|
This was written alongside #2870, which completely changes the way these are used. They were just separated into "needs compat exclusions" and not PRs |
This PR turns properties into records, for current & future performance improvements with JVM optimisations (and eventual value class conversion).
This is theoretically a breaking change, but not within general API surface area. It's a break to anything that was extending AbstractProperty creating their own Property type, which in general shouldn't really be done- so this change is viewed as acceptable.