Bind BlockTypes constants to explicit ids instead of field order - #3641
Open
MattBDev wants to merge 1 commit into
Open
Bind BlockTypes constants to explicit ids instead of field order#3641MattBDev wants to merge 1 commit into
MattBDev wants to merge 1 commit into
Conversation
BlockTypes 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 block. 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 BlockTypesCache to initialise moves into a static block above the constants, since only its position relative to them matters. That removes the per-call primed flag and the branch it guarded. Reconcile the constant list with upstream while here. Add DRIED_GHAST; drop POTTED_AZALEA and POTTED_FLOWERING_AZALEA, which upstream does not declare; and drop the fork-local deprecation of POTTED_AZALEA_BUSH and POTTED_FLOWERING_AZALEA_BUSH, whose comment pointed at the two constants being removed. The declarations are now identical to upstream's 1,201, with __RESERVED__ as the single deliberate addition the ordinal scheme depends on. 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.
SirYwell
requested changes
Sep 6, 2026
Comment on lines
+52
to
+58
| /** | ||
| * Placeholder occupying the null index, for use where block types are represented as primitives. | ||
| * | ||
| * <p>Bound to the synthetic id {@code minecraft:__reserved__}, which {@link BlockTypesCache} registers at internal id | ||
| * {@link BlockTypesCache.ReservedIDs#__RESERVED__} rather than obtaining from the platform. It is not a real block and | ||
| * must never be placed in the world.</p> | ||
| */ |
Member
There was a problem hiding this comment.
These are implementation details that shouldn't be part of the documentation
Contributor
Author
There was a problem hiding this comment.
I could just make this a comment instead of a javadoc if that's acceptable if we'd rather leave it without a doc?
| // Initializing the cache is what populates BlockType.REGISTRY from the running platform. Static initializers run in | ||
| // declaration order, so this must stay above the constants for them to resolve to anything. | ||
| //noinspection ResultOfMethodCallIgnored | ||
| BlockTypesCache.$NAMESPACES.isEmpty(); |
Member
There was a problem hiding this comment.
Maybe use MethodHandles.lookup().ensureInitialized(...) instead?
Contributor
Author
There was a problem hiding this comment.
🤔 I didn't know that existed. I'll look it up and change it if it works.
Comment on lines
+2477
to
+2483
| /** | ||
| * Parses user input into a block type, using a default parser context. | ||
| * | ||
| * @param type the input to parse, with or without a namespace and with or without a property specification | ||
| * @return the matching block type | ||
| * @throws InputParseException if the input matches no block type | ||
| */ |
Member
There was a problem hiding this comment.
The added documentation is unrelated, can you remove it?
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
BlockTypeshave led to falling behind updating constants for new Minecraft versions and to out-of-order constants. The reflective scheme used to bindthem to their ids is also fragile. This change removes the reflective scheme and matches the upstream
BlockTypesclass, which binds each constant to its id explicitly.Before, each block 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 block. This can lead to silent corruption of world data, which is a severe issue.
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
BlockTypeswrites every constant asget("minecraft:acacia_button"). 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 blocks 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
BlockType.REGISTRYtriggersBlockType's class initializer, notBlockTypesCache's, andREGISTRYstarts empty.BlockTypesCache's static block is what queries the platform and registers every block into it, so something has to force that class to initialize before the first constant resolves.That was the
BlockTypesCache.$NAMESPACES.isEmpty()call, guarded by acachePrimedflag so it ran once.Since
initno longer has lazy state of its own, 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. ThecachePrimedfield and the branch it guarded on every call are gone.BlockTypesCacheholds no reference back toBlockTypes, so there is no initialization cycle.Javadoc added to the public parsing and lookup methods (
parse,get,size, and friends), which previously had none.__RESERVED__gains a Javadoc explaining what it is and that it must never be placed in a world.Constant list reconciled with upstream
FAWE's list had drifted from WorldEdit's. This change brings it back into line:
DRIED_GHAST, declared upstream, absent from FAWEPOTTED_AZALEA,POTTED_FLOWERING_AZALEA, declared by FAWE, not by upstreamPOTTED_AZALEA_BUSH,POTTED_FLOWERING_AZALEA_BUSH, which carried a fork-local@Deprecated //No longer has "bush"that upstream does not have, and whose comment pointed at the two constants being removed__RESERVED__, FAWE-only and load-bearing: it occupies the null index so block types can be represented as primitivesThe block declarations now match upstream.
Performance
This touches
<clinit>only. It runs once per JVM, at plugin load.The one-time startup cost does improve slightly:
getDeclaredFields()walkFieldcopy for eachtoLowerCase(Locale.ROOT)SCREAMING_CASE, so none hit the identity fast path)"minecraft:" + keyallocations insideorDefaultNamespaceRoughly 6,000 transient allocations, all of them 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 about 42 KB to
BlockTypes.classand roughly 85 KB of memory that sticks around permanently. But 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.