Skip to content

Conversation

@me4502
Copy link
Member

@me4502 me4502 commented Dec 31, 2025

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.

@me4502 me4502 requested a review from a team as a code owner December 31, 2025 07:31
@me4502 me4502 merged commit 4bdd11d into master Jan 1, 2026
5 checks passed
@me4502 me4502 deleted the perf/make-properties-records branch January 1, 2026 04:52
@SirYwell
Copy link
Contributor

SirYwell commented Jan 2, 2026

Note that this makes subsequent changes to any of the fields impossible (e.g., using an (Enum)Set in DirectionalProperty) while not providing any actual performance benefit.

@octylFractal
Copy link
Member

Perhaps it would be worth doing that optimization now then, but considering we have to maintain the List contract anyway making it a Set is still rather involved even before this change. We're planning to do WE 8 after 7.4.x becomes mainline, so breaking this would not be a huge deal.

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.

@me4502
Copy link
Member Author

me4502 commented Jan 2, 2026

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.

@SirYwell
Copy link
Contributor

SirYwell commented Jan 2, 2026

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.

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 AbstractProperty also only uses the name for the hash code, now the list will be included as well.

@me4502
Copy link
Member Author

me4502 commented Jan 2, 2026

This was written alongside #2870, which completely changes the way these are used. They were just separated into "needs compat exclusions" and not PRs

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 this pull request may close these issues.

3 participants