From cdf5fc93ffaf4727f9a3b5001bd4450412304ab7 Mon Sep 17 00:00:00 2001 From: Madeline Miller Date: Sun, 7 Jul 2024 15:40:36 +1000 Subject: [PATCH 1/4] Refactor PlatformManager to use locks --- .../extension/platform/PlatformManager.java | 133 ++++++++++++------ 1 file changed, 93 insertions(+), 40 deletions(-) diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java index 0561b2b9f8..61f65a619d 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java @@ -53,6 +53,8 @@ import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.annotation.Nullable; import static com.google.common.base.Preconditions.checkNotNull; @@ -75,6 +77,9 @@ public class PlatformManager { private final AtomicBoolean initialized = new AtomicBoolean(); private final AtomicBoolean configured = new AtomicBoolean(); + private final ReadWriteLock platformsLock = new ReentrantReadWriteLock(); + private final ReadWriteLock preferencesLock = new ReentrantReadWriteLock(); + /** * Create a new platform manager. * @@ -94,14 +99,19 @@ public PlatformManager(WorldEdit worldEdit) { * * @param platform the platform */ - public synchronized void register(Platform platform) { + public void register(Platform platform) { checkNotNull(platform); LOGGER.info("Got request to register " + platform.getClass() + " with WorldEdit [" + super.toString() + "]"); // Just add the platform to the list of platforms: we'll pick favorites // once all the platforms have been loaded - platforms.add(platform); + platformsLock.writeLock().lock(); + try { + platforms.add(platform); + } finally { + platformsLock.writeLock().unlock(); + } // Make sure that versions are in sync if (firstSeenVersion != null) { @@ -123,26 +133,39 @@ public synchronized void register(Platform platform) { * * @param platform the platform */ - public synchronized boolean unregister(Platform platform) { + public boolean unregister(Platform platform) { checkNotNull(platform); - boolean removed = platforms.remove(platform); + boolean removed; + platformsLock.writeLock().lock(); + + try { + removed = platforms.remove(platform); + } finally { + platformsLock.writeLock().unlock(); + } if (removed) { LOGGER.info("Unregistering " + platform.getClass().getCanonicalName() + " from WorldEdit"); boolean choosePreferred = false; - // Check whether this platform was chosen to be the preferred one - // for any capability and be sure to remove it - Iterator> it = preferences.entrySet().iterator(); - while (it.hasNext()) { - Entry entry = it.next(); - if (entry.getValue().equals(platform)) { - entry.getKey().uninitialize(this, entry.getValue()); - it.remove(); - choosePreferred = true; // Have to choose new favorites + preferencesLock.writeLock().lock(); + + try { + // Check whether this platform was chosen to be the preferred one + // for any capability and be sure to remove it + Iterator> it = preferences.entrySet().iterator(); + while (it.hasNext()) { + Entry entry = it.next(); + if (entry.getValue().equals(platform)) { + entry.getKey().uninitialize(this, entry.getValue()); + it.remove(); + choosePreferred = true; // Have to choose new favorites + } } + } finally { + preferencesLock.writeLock().unlock(); } if (choosePreferred) { @@ -160,44 +183,60 @@ public synchronized boolean unregister(Platform platform) { * @return the platform * @throws NoCapablePlatformException thrown if no platform is capable */ - public synchronized Platform queryCapability(Capability capability) throws NoCapablePlatformException { - Platform platform = preferences.get(checkNotNull(capability)); - if (platform != null) { - return platform; - } else { - if (preferences.isEmpty()) { - // Not all platforms registered, this is being called too early! - throw new NoCapablePlatformException( - "Not all platforms have been registered yet!" - + " Please wait until WorldEdit is initialized." - ); + public Platform queryCapability(Capability capability) throws NoCapablePlatformException { + preferencesLock.readLock().lock(); + + try { + Platform platform = preferences.get(checkNotNull(capability)); + if (platform != null) { + return platform; + } else { + if (preferences.isEmpty()) { + // Not all platforms registered, this is being called too early! + throw new NoCapablePlatformException( + "Not all platforms have been registered yet!" + + " Please wait until WorldEdit is initialized." + ); + } + throw new NoCapablePlatformException("No platform was found supporting " + capability.name()); } - throw new NoCapablePlatformException("No platform was found supporting " + capability.name()); + } finally { + preferencesLock.readLock().unlock(); } } /** * Choose preferred platforms and perform necessary initialization. */ - private synchronized void choosePreferred() { + private void choosePreferred() { for (Capability capability : Capability.values()) { Platform preferred = findMostPreferred(capability); if (preferred != null) { - Platform oldPreferred = preferences.put(capability, preferred); - // only (re)initialize if it changed - if (preferred != oldPreferred) { - // uninitialize if needed - if (oldPreferred != null) { - capability.uninitialize(this, oldPreferred); + preferencesLock.writeLock().lock(); + try { + Platform oldPreferred = preferences.put(capability, preferred); + // only (re)initialize if it changed + if (preferred != oldPreferred) { + // uninitialize if needed + if (oldPreferred != null) { + capability.uninitialize(this, oldPreferred); + } + capability.initialize(this, preferred); } - capability.initialize(this, preferred); + } finally { + preferencesLock.writeLock().unlock(); } } } - // Fire configuration event - if (preferences.containsKey(Capability.CONFIGURATION) && configured.compareAndSet(false, true)) { - worldEdit.getEventBus().post(new ConfigurationLoadEvent(queryCapability(Capability.CONFIGURATION).getConfiguration())); + preferencesLock.readLock().lock(); + try { + // Fire configuration event + if (preferences.containsKey(Capability.CONFIGURATION) && configured.compareAndSet(false, true)) { + worldEdit.getEventBus().post(new ConfigurationLoadEvent(queryCapability(Capability.CONFIGURATION).getConfiguration())); + } + } finally { + preferencesLock.readLock().unlock(); } } @@ -208,11 +247,20 @@ private synchronized void choosePreferred() { * @param capability the capability * @return the most preferred platform, or null if no platform was found */ - private synchronized @Nullable Platform findMostPreferred(Capability capability) { + private @Nullable Platform findMostPreferred(Capability capability) { Platform preferred = null; Preference highest = null; - for (Platform platform : platforms) { + List availablePlatforms; + platformsLock.readLock().lock(); + + try { + availablePlatforms = getPlatforms(); + } finally { + platformsLock.readLock().unlock(); + } + + for (Platform platform : availablePlatforms) { Preference preference = platform.getCapabilities().get(capability); if (preference != null && (highest == null || preference.isPreferredOver(highest))) { preferred = platform; @@ -230,8 +278,13 @@ private synchronized void choosePreferred() { * * @return a list of platforms */ - public synchronized List getPlatforms() { - return new ArrayList<>(platforms); + public List getPlatforms() { + platformsLock.readLock().lock(); + try { + return new ArrayList<>(platforms); + } finally { + platformsLock.readLock().unlock(); + } } /** From 0ac7a041077246bfb6ed1664e5b4c3b93e0af22a Mon Sep 17 00:00:00 2001 From: Madeline Miller Date: Sun, 7 Jul 2024 15:59:11 +1000 Subject: [PATCH 2/4] fix a case of unnecessary reentrant locking --- .../worldedit/extension/platform/PlatformManager.java | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java index 61f65a619d..ce85d1208f 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java @@ -251,16 +251,7 @@ private void choosePreferred() { Platform preferred = null; Preference highest = null; - List availablePlatforms; - platformsLock.readLock().lock(); - - try { - availablePlatforms = getPlatforms(); - } finally { - platformsLock.readLock().unlock(); - } - - for (Platform platform : availablePlatforms) { + for (Platform platform : getPlatforms()) { Preference preference = platform.getCapabilities().get(capability); if (preference != null && (highest == null || preference.isPreferredOver(highest))) { preferred = platform; From a27bdf232cd12314cad5b76f0f463d468ef76cc7 Mon Sep 17 00:00:00 2001 From: Madeline Miller Date: Sun, 7 Jul 2024 16:10:30 +1000 Subject: [PATCH 3/4] keep the platform lock until preferences are updated in #unregister --- .../extension/platform/PlatformManager.java | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java index ce85d1208f..f5118329ea 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java @@ -141,36 +141,36 @@ public boolean unregister(Platform platform) { try { removed = platforms.remove(platform); - } finally { - platformsLock.writeLock().unlock(); - } - if (removed) { - LOGGER.info("Unregistering " + platform.getClass().getCanonicalName() + " from WorldEdit"); + if (removed) { + LOGGER.info("Unregistering " + platform.getClass().getCanonicalName() + " from WorldEdit"); - boolean choosePreferred = false; + boolean choosePreferred = false; - preferencesLock.writeLock().lock(); + preferencesLock.writeLock().lock(); - try { - // Check whether this platform was chosen to be the preferred one - // for any capability and be sure to remove it - Iterator> it = preferences.entrySet().iterator(); - while (it.hasNext()) { - Entry entry = it.next(); - if (entry.getValue().equals(platform)) { - entry.getKey().uninitialize(this, entry.getValue()); - it.remove(); - choosePreferred = true; // Have to choose new favorites + try { + // Check whether this platform was chosen to be the preferred one + // for any capability and be sure to remove it + Iterator> it = preferences.entrySet().iterator(); + while (it.hasNext()) { + Entry entry = it.next(); + if (entry.getValue().equals(platform)) { + entry.getKey().uninitialize(this, entry.getValue()); + it.remove(); + choosePreferred = true; // Have to choose new favorites + } } + } finally { + preferencesLock.writeLock().unlock(); } - } finally { - preferencesLock.writeLock().unlock(); - } - if (choosePreferred) { - choosePreferred(); + if (choosePreferred) { + choosePreferred(); + } } + } finally { + platformsLock.writeLock().unlock(); } return removed; From 83f7b2b505584a1dc415145e9800cb9914ec200c Mon Sep 17 00:00:00 2001 From: Madeline Miller Date: Sun, 7 Jul 2024 16:58:15 +1000 Subject: [PATCH 4/4] Use StampedLocks for performance reasons --- .../extension/platform/PlatformManager.java | 156 ++++++++++-------- 1 file changed, 88 insertions(+), 68 deletions(-) diff --git a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java index f5118329ea..9f43bcf1ee 100644 --- a/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java +++ b/worldedit-core/src/main/java/com/sk89q/worldedit/extension/platform/PlatformManager.java @@ -53,8 +53,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.StampedLock; import javax.annotation.Nullable; import static com.google.common.base.Preconditions.checkNotNull; @@ -77,8 +76,8 @@ public class PlatformManager { private final AtomicBoolean initialized = new AtomicBoolean(); private final AtomicBoolean configured = new AtomicBoolean(); - private final ReadWriteLock platformsLock = new ReentrantReadWriteLock(); - private final ReadWriteLock preferencesLock = new ReentrantReadWriteLock(); + private final StampedLock platformsLock = new StampedLock(); + private final StampedLock preferencesLock = new StampedLock(); /** * Create a new platform manager. @@ -106,11 +105,11 @@ public void register(Platform platform) { // Just add the platform to the list of platforms: we'll pick favorites // once all the platforms have been loaded - platformsLock.writeLock().lock(); + long stamp = platformsLock.writeLock(); try { platforms.add(platform); } finally { - platformsLock.writeLock().unlock(); + platformsLock.unlockWrite(stamp); } // Make sure that versions are in sync @@ -137,40 +136,40 @@ public boolean unregister(Platform platform) { checkNotNull(platform); boolean removed; - platformsLock.writeLock().lock(); + long platformsStamp = platformsLock.writeLock(); try { removed = platforms.remove(platform); + } finally { + platformsLock.unlockWrite(platformsStamp); + } - if (removed) { - LOGGER.info("Unregistering " + platform.getClass().getCanonicalName() + " from WorldEdit"); + if (removed) { + LOGGER.info("Unregistering " + platform.getClass().getCanonicalName() + " from WorldEdit"); - boolean choosePreferred = false; + boolean choosePreferred = false; - preferencesLock.writeLock().lock(); + long preferencesStamp = preferencesLock.writeLock(); - try { - // Check whether this platform was chosen to be the preferred one - // for any capability and be sure to remove it - Iterator> it = preferences.entrySet().iterator(); - while (it.hasNext()) { - Entry entry = it.next(); - if (entry.getValue().equals(platform)) { - entry.getKey().uninitialize(this, entry.getValue()); - it.remove(); - choosePreferred = true; // Have to choose new favorites - } + try { + // Check whether this platform was chosen to be the preferred one + // for any capability and be sure to remove it + Iterator> it = preferences.entrySet().iterator(); + while (it.hasNext()) { + Entry entry = it.next(); + if (entry.getValue().equals(platform)) { + entry.getKey().uninitialize(this, entry.getValue()); + it.remove(); + choosePreferred = true; // Have to choose new favorites } - } finally { - preferencesLock.writeLock().unlock(); } + } finally { + preferencesLock.unlockWrite(preferencesStamp); + } - if (choosePreferred) { - choosePreferred(); - } + if (choosePreferred) { + choosePreferred(); } - } finally { - platformsLock.writeLock().unlock(); } return removed; @@ -184,24 +183,33 @@ public boolean unregister(Platform platform) { * @throws NoCapablePlatformException thrown if no platform is capable */ public Platform queryCapability(Capability capability) throws NoCapablePlatformException { - preferencesLock.readLock().lock(); + checkNotNull(capability); + + long stamp = preferencesLock.tryOptimisticRead(); + Platform platform = preferences.get(capability); + boolean hasNoPreferences = platform == null && preferences.isEmpty(); + + if (!preferencesLock.validate(stamp)) { + stamp = preferencesLock.readLock(); + try { + platform = preferences.get(capability); + hasNoPreferences = platform == null && preferences.isEmpty(); + } finally { + preferencesLock.unlockRead(stamp); + } + } - try { - Platform platform = preferences.get(checkNotNull(capability)); - if (platform != null) { - return platform; - } else { - if (preferences.isEmpty()) { - // Not all platforms registered, this is being called too early! - throw new NoCapablePlatformException( - "Not all platforms have been registered yet!" - + " Please wait until WorldEdit is initialized." - ); - } - throw new NoCapablePlatformException("No platform was found supporting " + capability.name()); + if (platform != null) { + return platform; + } else { + if (hasNoPreferences) { + // Not all platforms registered, this is being called too early! + throw new NoCapablePlatformException( + "Not all platforms have been registered yet!" + + " Please wait until WorldEdit is initialized." + ); } - } finally { - preferencesLock.readLock().unlock(); + throw new NoCapablePlatformException("No platform was found supporting " + capability.name()); } } @@ -212,31 +220,37 @@ private void choosePreferred() { for (Capability capability : Capability.values()) { Platform preferred = findMostPreferred(capability); if (preferred != null) { - preferencesLock.writeLock().lock(); + Platform oldPreferred; + long stamp = preferencesLock.writeLock(); try { - Platform oldPreferred = preferences.put(capability, preferred); - // only (re)initialize if it changed - if (preferred != oldPreferred) { - // uninitialize if needed - if (oldPreferred != null) { - capability.uninitialize(this, oldPreferred); - } - capability.initialize(this, preferred); - } + oldPreferred = preferences.put(capability, preferred); } finally { - preferencesLock.writeLock().unlock(); + preferencesLock.unlockWrite(stamp); + } + // only (re)initialize if it changed + if (preferred != oldPreferred) { + // uninitialize if needed + if (oldPreferred != null) { + capability.uninitialize(this, oldPreferred); + } + capability.initialize(this, preferred); } } } - preferencesLock.readLock().lock(); - try { - // Fire configuration event - if (preferences.containsKey(Capability.CONFIGURATION) && configured.compareAndSet(false, true)) { - worldEdit.getEventBus().post(new ConfigurationLoadEvent(queryCapability(Capability.CONFIGURATION).getConfiguration())); + long stamp = preferencesLock.tryOptimisticRead(); + boolean hasConfiguration = preferences.containsKey(Capability.CONFIGURATION); + if (!preferencesLock.validate(stamp)) { + stamp = preferencesLock.readLock(); + try { + hasConfiguration = preferences.containsKey(Capability.CONFIGURATION); + } finally { + preferencesLock.unlockRead(stamp); } - } finally { - preferencesLock.readLock().unlock(); + } + // Fire configuration event + if (hasConfiguration && configured.compareAndSet(false, true)) { + worldEdit.getEventBus().post(new ConfigurationLoadEvent(queryCapability(Capability.CONFIGURATION).getConfiguration())); } } @@ -270,12 +284,18 @@ private void choosePreferred() { * @return a list of platforms */ public List getPlatforms() { - platformsLock.readLock().lock(); - try { - return new ArrayList<>(platforms); - } finally { - platformsLock.readLock().unlock(); + long stamp = platformsLock.tryOptimisticRead(); + List platformsCopy = new ArrayList<>(platforms); + if (!platformsLock.validate(stamp)) { + stamp = platformsLock.readLock(); + try { + platformsCopy = new ArrayList<>(platforms); + } finally { + platformsLock.unlockRead(stamp); + } } + + return platformsCopy; } /**