[1.19.2] Performance improvement and thread-safety review on NetworkRegistry
Forge Version: 1.19.2-43.1.3
Introduction:
Currently, net.minecraftforge.network.NetworkRegistry uses the following declaration:
private static Map<ResourceLocation, NetworkInstance> instances = Collections.synchronizedMap(new HashMap());I think we'd better replace it with
private static final Map<ResourceLocation, NetworkInstance> instances = new ConcurrentHashMap<>();The performance of Java's ConcurrentHashMap is much higher than that of a synchronized map. And this map only needs a lock during registration. After #lock(), the mutex lock is just a burden, although network packets are now processed on the Netty thread.
Besides that, #createInstance() is not thread-safe when checking for registry duplicates. Checking Map#containsKey() and then Map#put() is not an atomic operation, there may be a race here. Benefitting from ConcurrentHashMap, we could use:
NetworkInstance networkInstance = new NetworkInstance(name, networkProtocolVersion, clientAcceptedVersions, serverAcceptedVersions);
NetworkInstance old = instances.put(name, networkInstance);
if (old != null) {
throw new IllegalArgumentException("NetworkDirection Channel {" + name + "} already registered");
}Also, the #lock field should be volatile, since #createInstance() may be called from any thread at any time.
Source: MinecraftForge/MinecraftForge