Minor changes to MixinMapGenStructure. - #932
Conversation
- Replace field with `@Share` for thread-safety. - Replace `@WrapOperation` with `@Redirect`, as `original` was never being used. - Replaced `Long2ObjectOpenHashMap` cast with a cast to the more generic `Long2ObjectMap` for inter-mod compatibility
…spotless I guess...
| 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); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, what class do you use instead of Long2ObjectOpenHashMap? Do you have some kind of custom implementation of a ConcurrentHashMap for this?
There was a problem hiding this comment.
It's a custom implementation of a Read-Write locked Long2ObjectMap<T>. A similar replacement was already performed in #930.
| Operation<Boolean> original) { | ||
| return ((Long2ObjectOpenHashMap<StructureStart>) structureMap).containsKey(hodgepodge$localRef); | ||
| @Share("unboxedLong") LocalLongRef unboxedLong) { | ||
| return ((Long2ObjectMap<StructureStart>) structureMap).containsKey(unboxedLong.get()); |
There was a problem hiding this comment.
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.)
2 participants
@Sharefor thread-safety.@WrapOperationwith@Redirect, asoriginalwas never being used.Long2ObjectOpenHashMapcast with a cast to the more genericLong2ObjectMapfor inter-mod compatibility