Skip to content

Minor changes to MixinMapGenStructure. #932

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.

Sign up for GitHub

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

Open
BallOfEnergy1 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Minor changes to MixinMapGenStructure. #932

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b20a7d7
Minor changes to `MixinMapGenStructure`.
BallOfEnergy1 Jun 24, 2026
4dadfe5
Running spotlessApply just sometimes decides it doesnt want to apply …
BallOfEnergy1 Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions .../java/com/mitchej123/hodgepodge/mixins/early/minecraft/fastload/MixinMapGenStructure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Redirect;

import com.llamalad7.mixinextras.injector.wrapoperation.Operation;
import com.llamalad7.mixinextras.injector.wrapoperation.WrapOperation;
import com.llamalad7.mixinextras.sugar.Share;
import com.llamalad7.mixinextras.sugar.ref.LocalLongRef;

import it.unimi.dsi.fastutil.longs.Long2ObjectMap;
import it.unimi.dsi.fastutil.longs.Long2ObjectOpenHashMap;

@Mixin(MapGenStructure.class)
Expand All @@ -21,22 +22,20 @@ public class MixinMapGenStructure {
@Shadow
protected Map<Long, StructureStart> structureMap = new Long2ObjectOpenHashMap<>();

private long hodgepodge$localRef;

@Redirect(
method = "func_151538_a",
at = @At(value = "INVOKE", target = "Ljava/lang/Long;valueOf(J)Ljava/lang/Long;"))
private Long hodgepodge$nukeBox(long l) {
hodgepodge$localRef = l;
private Long hodgepodge$nukeBox(long l, @Share("unboxedLong") LocalLongRef unboxedLong) {
unboxedLong.set(l);
return null;
}

@WrapOperation(
@Redirect(
method = "func_151538_a",
at = @At(value = "INVOKE", target = "Ljava/util/Map;containsKey(Ljava/lang/Object;)Z"))
private boolean hodgepodge$primitiveContains(Map<Long, StructureStart> instance, Object o,
Operation<Boolean> original) {
return ((Long2ObjectOpenHashMap<StructureStart>) structureMap).containsKey(hodgepodge$localRef);
@Share("unboxedLong") LocalLongRef unboxedLong) {
return ((Long2ObjectMap<StructureStart>) structureMap).containsKey(unboxedLong.get());

@Nikolay-Sitnikov Nikolay-Sitnikov Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see a potential problem. This world generator checks containsKey, and if the key is missing, it uses put to create that key. However, any naive concurrent map implementation would treat these two operations as independent, thus allowing for two threads to both detect that the map is missing the key and both attempt to generate that structure at the same time.

Since I assume you don't want that, you might need to re-implement this mixin yourself in order to handle this edge-case. (By locking the map on the first call and only unlocking it after the put, or creating the key with an atomic computeIfAbsent to add a placeholder, or by moving the rest of the method into the computeIfAbsent call using @WrapMethod.)

}

@Redirect(
Expand All @@ -45,7 +44,8 @@ public class MixinMapGenStructure {
value = "INVOKE",
target = "Ljava/util/Map;put(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;",
ordinal = 0))
private Object hodgepodge$primitiveContains(Map<Long, StructureStart> instance, Object k, Object v) {
return ((Long2ObjectOpenHashMap<StructureStart>) structureMap).put(hodgepodge$localRef, (StructureStart) v);
private Object hodgepodge$primitiveContains(Map<Long, StructureStart> instance, Object k, Object v,
@Share("unboxedLong") LocalLongRef unboxedLong) {
return ((Long2ObjectMap<StructureStart>) structureMap).put(unboxedLong.get(), (StructureStart) v);

@Nikolay-Sitnikov Nikolay-Sitnikov Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think changing the cast of structureMap is a good idea, given that Long2ObjectOpenHashMap is the best map to use in this circumstance, and that this cast allows us to perform a non-interface virtual call (which is faster than an interface virtual call.)

@BallOfEnergy1 BallOfEnergy1 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose behind changing this to a more generic cast is for mod compatibility, as one of my mods (Spool) needs to synchronize this map. The performance difference does exist, but it should be negligible in this code as it's not a particularly hot spot.

@Nikolay-Sitnikov Nikolay-Sitnikov Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, what class do you use instead of Long2ObjectOpenHashMap? Do you have some kind of custom implementation of a ConcurrentHashMap for this?

@BallOfEnergy1 BallOfEnergy1 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a custom implementation of a Read-Write locked Long2ObjectMap<T>. A similar replacement was already performed in #930.

}
}