Bind ItemTypes constants to explicit ids instead of field order - #3642
Open
MattBDev wants to merge 1 commit into
Open
Bind ItemTypes constants to explicit ids instead of field order#3642MattBDev wants to merge 1 commit into
MattBDev wants to merge 1 commit into
Conversation
ItemTypes resolved each of its constants by reading its own field list reflectively and lowercasing the field name at the matching index. Class.getDeclaredFields() explicitly does not specify an order, so this depended on undefined behaviour; a sentinel static block was the only thing standing between a reordered array and every constant silently bound to the wrong item. Each constant now calls the existing get(String) lookup directly with its namespaced id, which is also the scheme upstream WorldEdit already uses. Field order carries no meaning, so the sentinel block, the fieldsTmp/initIndex state, the reflective walk, and the intermediate init(String) wrapper are all gone. Forcing ItemTypesCache to initialise moves into a static block above the constants, since only its position relative to them matters. That removes the lazy fieldsTmp == null check that previously guarded the one-time call. Reconcile the constant list with upstream while here. FAWE had fallen 19 constants behind: BLACK_HARNESS, BLUE_HARNESS, BROWN_HARNESS, CYAN_HARNESS, DRIED_GHAST, GRAY_HARNESS, GREEN_HARNESS, HAPPY_GHAST_SPAWN_EGG, LIGHT_BLUE_HARNESS, LIGHT_GRAY_HARNESS, LIME_HARNESS, MAGENTA_HARNESS, MUSIC_DISC_TEARS, ORANGE_HARNESS, PINK_HARNESS, PURPLE_HARNESS, RED_HARNESS, WHITE_HARNESS, and YELLOW_HARNESS are all declared upstream and were missing here. No constant existed in FAWE that upstream lacks, so nothing was removed. The declarations are now identical to upstream's 1,546. Every id is "minecraft:" + the lowercased field name, matching both what the previous scheme produced and what upstream declares, so the resolved values are unchanged for every constant that survives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Our changes to
ItemTypeshave the same problemBlockTypeshad (#3641): falling behind updating constants for new Minecraft versions, and a reflective scheme for binding constants to their ids that's fragile. This change removes the reflective scheme and matches the upstreamItemTypesclass, which binds each constant to its id explicitly.Before, each item constant called a no-argument
init()that walked the class's own field array and derivedthe id from whichever field happened to sit at the next index:
Each constant now states its id and uses the existing
get(String)method to resolve it:No new method was introduced.
Why
The biggest problem is that constant resolution relies on
Class.getDeclaredFields(). This method doesnot specify an order. We just happened to luck into the fact that the JVM returned fields in source order, but that is not
guaranteed and we shouldn't be left relying on that.
The consequence of this is that every constant from the first disturbance
onward is bound to the wrong item. This can lead to silent corruption of world data, which is a severe issue.
The old code did have a check for this, but it only covers one kind of mix-up (missing/extra fields). It wouldn't catch two items just being swapped, since the count would still look right.
Writing the id straight into each constant just makes the whole problem impossible instead of trying to catch it after the fact.
Upstream already does it this way. WorldEdit's own
ItemTypeswrites every constant asget("minecraft:acacia_boat"). We were doing our own thing here for no real reason; now we're not, so pulling in upstream changes to this file gets a lot easier.If we fall behind updating Minecraft items in the future, we should be able to just cherry-pick the upstream change.
Also in this change
Cache priming moved to a static block. Reading
ItemType.REGISTRYtriggersItemType's class initializer, notItemTypesCache's, andREGISTRYstarts empty.ItemTypesCache's static block is what queries the platform and registers every item into it, so something has to force that class to initialize before the first constant resolves.That was the
ItemTypesCache.init()call, guarded by a lazyfieldsTmp == nullcheck so it ran once.Since there's no lazy state left to guard, the priming moves to a static block placed above the constants:
Static blocks and field initializers compile into the same
<clinit>in source order, so this runs first and exactly once.Constant list reconciled with upstream
FAWE's list had drifted from WorldEdit's. This change brings it back into line by adding the 19 constants FAWE was missing:
BLACK_HARNESS,BLUE_HARNESS,BROWN_HARNESS,CYAN_HARNESS,DRIED_GHAST,GRAY_HARNESS,GREEN_HARNESS,HAPPY_GHAST_SPAWN_EGG,LIGHT_BLUE_HARNESS,LIGHT_GRAY_HARNESS,LIME_HARNESS,MAGENTA_HARNESS,MUSIC_DISC_TEARS,ORANGE_HARNESS,PINK_HARNESS,PURPLE_HARNESS,RED_HARNESS,WHITE_HARNESS,YELLOW_HARNESSPerformance
This touches
<clinit>only. It runs once per JVM, at plugin load.The one-time startup cost does improve slightly: gone is a native metadata walk over a ~1,550-field class, one
toLowerCase(Locale.ROOT)allocation per constant, and one namespace-concatenation allocation per constant. That's a few thousand transient allocations, all young-gen garbage, at a moment when the server is already doing far more expensive work. It is not measurable in practice.The tradeoff: we are now adding item id strings baked into the class file, adding a modest amount of permanent memory. The old version wasn't free either. It kept a full copy of the class's fields cached in memory the whole time, which was a similar amount of memory. So the two roughly cancel out, and either way it's nothing on a server with gigabytes of heap.