mirror of
https://github.com/PaperMC/Paper.git
synced 2024-12-28 15:30:19 +01:00
Synchronize DataPaletteBlock instead of ReentrantLock
Mojang has flaws in their logic about chunks being concurrently wrote to. So we constantly see crashes around multiple threads writing. Additionally, java has optimized synchronization so well that its in many times faster than trying to manage read wrote locks for low contention situations. And this is extremely a low contention situation. Fixes #3293 Fixes #2493
This commit is contained in:
parent
a8ef0a93b9
commit
7a2b345b55
4 changed files with 111 additions and 58 deletions
|
@ -1,48 +0,0 @@
|
|||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
||||
From: Spottedleaf <Spottedleaf@users.noreply.github.com>
|
||||
Date: Fri, 21 Jun 2019 14:42:48 -0700
|
||||
Subject: [PATCH] Log other thread in DataPaletteBlock lock failure
|
||||
|
||||
|
||||
diff --git a/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java b/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java
|
||||
new file mode 100644
|
||||
index 0000000000000000000000000000000000000000..a3b174618d4947bc1f1b94df8dd4fe2ccf719fe9
|
||||
--- /dev/null
|
||||
+++ b/src/main/java/com/destroystokyo/paper/util/ReentrantLockWithGetOwner.java
|
||||
@@ -0,0 +1,11 @@
|
||||
+package com.destroystokyo.paper.util;
|
||||
+
|
||||
+import java.util.concurrent.locks.ReentrantLock;
|
||||
+
|
||||
+public class ReentrantLockWithGetOwner extends ReentrantLock {
|
||||
+
|
||||
+ @Override
|
||||
+ public Thread getOwner() {
|
||||
+ return super.getOwner();
|
||||
+ }
|
||||
+}
|
||||
diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
index d5f5a51872dfabdbb828b6c20d61893aed2efec7..2c1d1b1a5568ab104fb1a0f283f289680d59e81e 100644
|
||||
--- a/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
+++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
@@ -22,14 +22,17 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
protected DataBits a; protected DataBits getDataBits() { return this.a; } // Paper - OBFHELPER
|
||||
private DataPalette<T> h; private DataPalette<T> getDataPalette() { return this.h; } // Paper - OBFHELPER
|
||||
private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER
|
||||
- private final ReentrantLock j = new ReentrantLock();
|
||||
+ private final com.destroystokyo.paper.util.ReentrantLockWithGetOwner j = new com.destroystokyo.paper.util.ReentrantLockWithGetOwner(); private com.destroystokyo.paper.util.ReentrantLockWithGetOwner getLock() { return this.j; } // Paper - change type to ReentrantLockWithGetOwner // Paper - OBFHELPER
|
||||
|
||||
public void a() {
|
||||
- if (this.j.isLocked() && !this.j.isHeldByCurrentThread()) {
|
||||
+ // Paper start - log other thread
|
||||
+ Thread owningThread;
|
||||
+ if (this.j.isLocked() && (owningThread = this.getLock().getOwner()) != null && owningThread != Thread.currentThread()) {
|
||||
+ // Paper end
|
||||
String s = (String) Thread.getAllStackTraces().keySet().stream().filter(Objects::nonNull).map((thread) -> {
|
||||
return thread.getName() + ": \n\tat " + (String) Arrays.stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n\tat "));
|
||||
}).collect(Collectors.joining("\n"));
|
||||
- CrashReport crashreport = new CrashReport("Writing into PalettedContainer from multiple threads", new IllegalStateException());
|
||||
+ CrashReport crashreport = new CrashReport("Writing into PalettedContainer from multiple threads (other thread: name: " + owningThread.getName() + ", class: " + owningThread.getClass().toString() + ")", new IllegalStateException()); // Paper - log other thread
|
||||
CrashReportSystemDetails crashreportsystemdetails = crashreport.a("Thread dumps");
|
||||
|
||||
crashreportsystemdetails.a("Thread dumps", (Object) s);
|
|
@ -0,0 +1,101 @@
|
|||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
|
||||
From: Aikar <aikar@aikar.co>
|
||||
Date: Fri, 29 May 2020 20:29:02 -0400
|
||||
Subject: [PATCH] Synchronize DataPaletteBlock instead of ReentrantLock
|
||||
|
||||
Mojang has flaws in their logic about chunks being concurrently
|
||||
wrote to. So we constantly see crashes around multiple threads writing.
|
||||
|
||||
Additionally, java has optimized synchronization so well that its
|
||||
in many times faster than trying to manage read wrote locks for low
|
||||
contention situations.
|
||||
|
||||
And this is extremely a low contention situation.
|
||||
|
||||
diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
index d5f5a51872dfabdbb828b6c20d61893aed2efec7..3da575929fcf878ebfe18e3240fac84325159225 100644
|
||||
--- a/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
+++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
@@ -24,7 +24,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER
|
||||
private final ReentrantLock j = new ReentrantLock();
|
||||
|
||||
- public void a() {
|
||||
+ public void a() { /* // Paper start - disable this - use proper synchronization
|
||||
if (this.j.isLocked() && !this.j.isHeldByCurrentThread()) {
|
||||
String s = (String) Thread.getAllStackTraces().keySet().stream().filter(Objects::nonNull).map((thread) -> {
|
||||
return thread.getName() + ": \n\tat " + (String) Arrays.stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n\tat "));
|
||||
@@ -36,11 +36,11 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
throw new ReportedException(crashreport);
|
||||
} else {
|
||||
this.j.lock();
|
||||
- }
|
||||
+ } */ // Paper end
|
||||
}
|
||||
|
||||
public void b() {
|
||||
- this.j.unlock();
|
||||
+ //this.j.unlock(); // Paper - disable this
|
||||
}
|
||||
|
||||
public DataPaletteBlock(DataPalette<T> datapalette, RegistryBlockID<T> registryblockid, Function<NBTTagCompound, T> function, Function<T, NBTTagCompound> function1, T t0) {
|
||||
@@ -76,7 +76,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
}
|
||||
|
||||
@Override
|
||||
- public int onResize(int i, T t0) {
|
||||
+ public synchronized int onResize(int i, T t0) { // Paper - synchronize
|
||||
this.a();
|
||||
DataBits databits = this.a;
|
||||
DataPalette<T> datapalette = this.h;
|
||||
@@ -99,18 +99,18 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
}
|
||||
|
||||
public T setBlock(int i, int j, int k, T t0) {
|
||||
- this.a();
|
||||
- T t1 = this.a(b(i, j, k), t0);
|
||||
+ //this.a(); // Paper - remove to reduce ops - synchronize handled below
|
||||
+ return this.a(b(i, j, k), t0); // Paper
|
||||
|
||||
- this.b();
|
||||
- return t1;
|
||||
+ //this.b(); // Paper
|
||||
+ //return t1; // PAper
|
||||
}
|
||||
|
||||
public T b(int i, int j, int k, T t0) {
|
||||
return this.a(b(i, j, k), t0);
|
||||
}
|
||||
|
||||
- protected T a(int i, T t0) {
|
||||
+ protected synchronized T a(int i, T t0) { // Paper - synchronize - writes
|
||||
int j = this.h.a(t0);
|
||||
int k = this.a.a(i, j);
|
||||
T t1 = this.h.a(k);
|
||||
@@ -135,7 +135,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
}
|
||||
|
||||
public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // Paper - OBFHELPER
|
||||
- public void b(PacketDataSerializer packetdataserializer) {
|
||||
+ public synchronized void b(PacketDataSerializer packetdataserializer) { // Paper - synchronize
|
||||
this.a();
|
||||
packetdataserializer.writeByte(this.i);
|
||||
this.h.b(packetdataserializer);
|
||||
@@ -143,7 +143,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
this.b();
|
||||
}
|
||||
|
||||
- public void a(NBTTagList nbttaglist, long[] along) {
|
||||
+ public synchronized void a(NBTTagList nbttaglist, long[] along) { // Paper - synchronize
|
||||
this.a();
|
||||
int i = Math.max(4, MathHelper.d(nbttaglist.size()));
|
||||
|
||||
@@ -176,7 +176,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
this.b();
|
||||
}
|
||||
|
||||
- public void a(NBTTagCompound nbttagcompound, String s, String s1) {
|
||||
+ public synchronized void a(NBTTagCompound nbttagcompound, String s, String s1) { // Paper - synchronize
|
||||
this.a();
|
||||
DataPaletteHash<T> datapalettehash = new DataPaletteHash<>(this.d, this.i, this.c, this.e, this.f);
|
||||
T t0 = this.g;
|
|
@ -1099,7 +1099,7 @@ index e056fbcb216977401fd2778fcd3ee7ed5f020214..eeb7eee925d31d1ed8e6bbd55888cb5e
|
|||
|
||||
public int j() {
|
||||
diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
index 2c1d1b1a5568ab104fb1a0f283f289680d59e81e..2c7872bd0516c820a42682b5281dbbaa1b467beb 100644
|
||||
index 3da575929fcf878ebfe18e3240fac84325159225..abe86b4607e014e280e29daae6e1617763c53131 100644
|
||||
--- a/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
+++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
@@ -3,6 +3,7 @@ package net.minecraft.server;
|
||||
|
@ -1118,8 +1118,8 @@ index 2c1d1b1a5568ab104fb1a0f283f289680d59e81e..2c7872bd0516c820a42682b5281dbbaa
|
|||
protected DataBits a; protected DataBits getDataBits() { return this.a; } // Paper - OBFHELPER
|
||||
private DataPalette<T> h; private DataPalette<T> getDataPalette() { return this.h; } // Paper - OBFHELPER
|
||||
private int i; private int getBitsPerObject() { return this.i; } // Paper - OBFHELPER
|
||||
@@ -46,14 +48,47 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
this.j.unlock();
|
||||
@@ -43,14 +45,47 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
//this.j.unlock(); // Paper - disable this
|
||||
}
|
||||
|
||||
- public DataPaletteBlock(DataPalette<T> datapalette, RegistryBlockID<T> registryblockid, Function<NBTTagCompound, T> function, Function<T, NBTTagCompound> function1, T t0) {
|
||||
|
@ -1168,7 +1168,7 @@ index 2c1d1b1a5568ab104fb1a0f283f289680d59e81e..2c7872bd0516c820a42682b5281dbbaa
|
|||
|
||||
private static int b(int i, int j, int k) {
|
||||
return j << 8 | k << 4 | i;
|
||||
@@ -88,6 +123,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
@@ -85,6 +120,7 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
|
||||
int j;
|
||||
|
||||
|
@ -1176,17 +1176,17 @@ index 2c1d1b1a5568ab104fb1a0f283f289680d59e81e..2c7872bd0516c820a42682b5281dbbaa
|
|||
for (j = 0; j < databits.b(); ++j) {
|
||||
T t1 = datapalette.a(databits.a(j));
|
||||
|
||||
@@ -137,24 +173,38 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
@@ -134,24 +170,38 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
return t0 == null ? this.g : t0;
|
||||
}
|
||||
|
||||
- public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // Paper - OBFHELPER
|
||||
- public void b(PacketDataSerializer packetdataserializer) {
|
||||
- public synchronized void b(PacketDataSerializer packetdataserializer) { // Paper - synchronize
|
||||
+ // Paper start - Anti-Xray - Add chunk packet info
|
||||
+ @Deprecated public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer) { this.b(packetDataSerializer); } // OBFHELPER // Notice for updates: Please make sure this method isn't used anywhere
|
||||
+ @Deprecated public void b(PacketDataSerializer packetdataserializer) { this.writeDataPaletteBlock(packetdataserializer, null, 0); } // Notice for updates: Please make sure this method isn't used anywhere
|
||||
+ public void writeDataPaletteBlock(PacketDataSerializer packetDataSerializer, ChunkPacketInfo<T> chunkPacketInfo, int chunkSectionIndex) { this.b(packetDataSerializer, chunkPacketInfo, chunkSectionIndex); } // OBFHELPER
|
||||
+ public void b(PacketDataSerializer packetdataserializer, ChunkPacketInfo<T> chunkPacketInfo, int chunkSectionIndex) {
|
||||
+ public synchronized void b(PacketDataSerializer packetdataserializer, ChunkPacketInfo<T> chunkPacketInfo, int chunkSectionIndex) { // Paper - synchronize
|
||||
+ // Paper end
|
||||
this.a();
|
||||
packetdataserializer.writeByte(this.i);
|
||||
|
@ -1203,7 +1203,7 @@ index 2c1d1b1a5568ab104fb1a0f283f289680d59e81e..2c7872bd0516c820a42682b5281dbbaa
|
|||
this.b();
|
||||
}
|
||||
|
||||
public void a(NBTTagList nbttaglist, long[] along) {
|
||||
public synchronized void a(NBTTagList nbttaglist, long[] along) { // Paper - synchronize
|
||||
this.a();
|
||||
- int i = Math.max(4, MathHelper.d(nbttaglist.size()));
|
||||
+ // Paper - Anti-Xray - TODO: Should this.predefinedObjects.length just be added here (faster) or should the contents be compared to calculate the size (less RAM)?
|
||||
|
|
|
@ -253,10 +253,10 @@ index f9680b6830c77f31e1eb8b6845dd6d58d04f624a..a61cffa3f494be5fea785a573b0faf05
|
|||
+ // Paper end
|
||||
}
|
||||
diff --git a/src/main/java/net/minecraft/server/DataPaletteBlock.java b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
index 2c7872bd0516c820a42682b5281dbbaa1b467beb..be5f98c3c368cb046112dc488344bb529fc395c1 100644
|
||||
index abe86b4607e014e280e29daae6e1617763c53131..c9a13c865b332a5b6dbea9f23c00b0c9d66f3798 100644
|
||||
--- a/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
+++ b/src/main/java/net/minecraft/server/DataPaletteBlock.java
|
||||
@@ -281,6 +281,14 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
@@ -278,6 +278,14 @@ public class DataPaletteBlock<T> implements DataPaletteExpandable<T> {
|
||||
});
|
||||
}
|
||||
|
||||
|
|
Loading…
Reference in a new issue