From 3a123f9fa1a3d9279f98ffdddf8b1217be211a2a Mon Sep 17 00:00:00 2001 From: Luck Date: Tue, 4 Sep 2018 21:02:40 +0100 Subject: [PATCH] Avoid running full "update tasks" unless they're absolutely needed. Process Vault API "set" requests immediately instead of in the background --- .../luckperms/bukkit/vault/VaultChatHook.java | 77 +++++++++---------- .../bukkit/vault/VaultPermissionHook.java | 72 ++++++++--------- .../command/utils/StorageAssistant.java | 6 +- .../managers/group/AbstractGroupManager.java | 6 ++ .../common/managers/group/GroupManager.java | 5 ++ .../managers/user/AbstractUserManager.java | 6 ++ .../common/managers/user/UserManager.java | 7 +- .../common/tasks/ExpireTemporaryTask.java | 3 +- .../luckperms/common/tasks/UpdateTask.java | 1 + .../service/internal/HolderSubjectData.java | 9 ++- 10 files changed, 105 insertions(+), 87 deletions(-) diff --git a/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultChatHook.java b/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultChatHook.java index 8ba9da73..e14775ae 100644 --- a/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultChatHook.java +++ b/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultChatHook.java @@ -50,11 +50,10 @@ import java.util.UUID; /** * An implementation of the Vault {@link Chat} API using LuckPerms. * - * Methods which change the state of data objects are likely to return immediately. - * * LuckPerms is a multithreaded permissions plugin, and some actions require considerable * time to execute. (database queries, re-population of caches, etc) In these cases, the - * methods will return immediately and the change will be executed asynchronously. + * operations required to make the edit apply will be processed immediately, but the process + * of saving the change to the plugin storage will happen in the background. * * Methods that have to query data from the database will throw exceptions when called * from the main thread. Users of the Vault API expect these methods to be "main thread friendly", @@ -251,27 +250,25 @@ public class VaultChatHook extends AbstractVaultChat { logMsg("#setChatMeta: %s - %s - %s - %s", holder.getFriendlyName(), type, value, world); } - this.permissionHook.getExecutor().execute(() -> { - // remove all prefixes/suffixes directly set on the user/group - holder.removeIf(type::matches); + // remove all prefixes/suffixes directly set on the user/group + holder.removeIf(type::matches); - if (value == null) { - this.permissionHook.holderSave(holder); - return; - } - - // find the max inherited priority & add 10 - MetaAccumulator metaAccumulator = holder.accumulateMeta(null, createContextForWorldSet(world)); - metaAccumulator.complete(); - int priority = metaAccumulator.getChatMeta(type).keySet().stream().mapToInt(e -> e).max().orElse(0) + 10; - - Node.Builder chatMetaNode = NodeFactory.buildChatMetaNode(type, priority, value); - chatMetaNode.setServer(this.permissionHook.getVaultServer()); - chatMetaNode.setWorld(world); - - holder.setPermission(chatMetaNode.build()); + if (value == null) { this.permissionHook.holderSave(holder); - }); + return; + } + + // find the max inherited priority & add 10 + MetaAccumulator metaAccumulator = holder.accumulateMeta(null, createContextForWorldSet(world)); + metaAccumulator.complete(); + int priority = metaAccumulator.getChatMeta(type).keySet().stream().mapToInt(e -> e).max().orElse(0) + 10; + + Node.Builder chatMetaNode = NodeFactory.buildChatMetaNode(type, priority, value); + chatMetaNode.setServer(this.permissionHook.getVaultServer()); + chatMetaNode.setWorld(world); + + holder.setPermission(chatMetaNode.build()); // assume success + this.permissionHook.holderSave(holder); } private void setMeta(PermissionHolder holder, String key, Object value, String world) { @@ -279,27 +276,25 @@ public class VaultChatHook extends AbstractVaultChat { logMsg("#setMeta: %s - %s - %s - %s", holder.getFriendlyName(), key, value, world); } - this.permissionHook.getExecutor().execute(() -> { - holder.removeIf(n -> n.isMeta() && n.getMeta().getKey().equals(key)); + holder.removeIf(n -> n.isMeta() && n.getMeta().getKey().equals(key)); - if (value == null) { - this.permissionHook.holderSave(holder); - return; - } - - Node.Builder metaNode; - if (key.equalsIgnoreCase(NodeTypes.PREFIX_KEY) || key.equalsIgnoreCase(NodeTypes.SUFFIX_KEY)) { - metaNode = NodeFactory.buildChatMetaNode(ChatMetaType.valueOf(key.toUpperCase()), 100, value.toString()); - } else { - metaNode = NodeFactory.buildMetaNode(key, value.toString()); - } - - metaNode.setServer(this.permissionHook.getVaultServer()); - metaNode.setWorld(world); - - holder.setPermission(metaNode.build()); + if (value == null) { this.permissionHook.holderSave(holder); - }); + return; + } + + Node.Builder metaNode; + if (key.equalsIgnoreCase(NodeTypes.PREFIX_KEY) || key.equalsIgnoreCase(NodeTypes.SUFFIX_KEY)) { + metaNode = NodeFactory.buildChatMetaNode(ChatMetaType.valueOf(key.toUpperCase()), 100, value.toString()); + } else { + metaNode = NodeFactory.buildMetaNode(key, value.toString()); + } + + metaNode.setServer(this.permissionHook.getVaultServer()); + metaNode.setWorld(world); + + holder.setPermission(metaNode.build()); // assume success + this.permissionHook.holderSave(holder); } private Contexts createContextForWorldSet(String world) { diff --git a/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultPermissionHook.java b/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultPermissionHook.java index 95e76b6d..a92ab54c 100644 --- a/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultPermissionHook.java +++ b/bukkit/src/main/java/me/lucko/luckperms/bukkit/vault/VaultPermissionHook.java @@ -51,17 +51,14 @@ import java.util.Arrays; import java.util.Objects; import java.util.Optional; import java.util.UUID; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; /** * An implementation of the Vault {@link Permission} API using LuckPerms. * - * Methods which change the state of data objects are likely to return immediately. - * * LuckPerms is a multithreaded permissions plugin, and some actions require considerable * time to execute. (database queries, re-population of caches, etc) In these cases, the - * methods will return immediately and the change will be executed asynchronously. + * operations required to make the edit apply will be processed immediately, but the process + * of saving the change to the plugin storage will happen in the background. * * Methods that have to query data from the database will throw exceptions when called * from the main thread. Users of the Vault API expect these methods to be "main thread friendly", @@ -73,12 +70,8 @@ public class VaultPermissionHook extends AbstractVaultPermission { // the plugin instance private final LPBukkitPlugin plugin; - // an executor for Vault modifications. - private final ExecutorService executor; - public VaultPermissionHook(LPBukkitPlugin plugin) { this.plugin = plugin; - this.executor = Executors.newSingleThreadExecutor(); this.worldMappingFunction = world -> isIgnoreWorld() ? null : world; } @@ -86,10 +79,6 @@ public class VaultPermissionHook extends AbstractVaultPermission { return this.plugin; } - public ExecutorService getExecutor() { - return this.executor; - } - @Override public String getName() { return "LuckPerms"; @@ -119,9 +108,9 @@ public class VaultPermissionHook extends AbstractVaultPermission { } // lookup a username from the database - UUID uuid = plugin.getStorage().getPlayerUuid(player.toLowerCase()).join(); + UUID uuid = this.plugin.getStorage().getPlayerUuid(player.toLowerCase()).join(); if (uuid == null) { - uuid = plugin.getBootstrap().lookupUuid(player).orElse(null); + uuid = this.plugin.getBootstrap().lookupUuid(player).orElse(null); } // unable to find a user, throw an exception @@ -186,8 +175,7 @@ public class VaultPermissionHook extends AbstractVaultPermission { Objects.requireNonNull(permission, "permission"); User user = lookupUser(uuid); - holderAddPermission(user, permission, world); - return true; + return holderAddPermission(user, permission, world); } @Override @@ -196,8 +184,7 @@ public class VaultPermissionHook extends AbstractVaultPermission { Objects.requireNonNull(permission, "permission"); User user = lookupUser(uuid); - holderRemovePermission(user, permission, world); - return true; + return holderRemovePermission(user, permission, world); } @Override @@ -295,8 +282,7 @@ public class VaultPermissionHook extends AbstractVaultPermission { return false; } - holderAddPermission(group, permission, world); - return true; + return holderAddPermission(group, permission, world); } @Override @@ -309,8 +295,7 @@ public class VaultPermissionHook extends AbstractVaultPermission { return false; } - holderRemovePermission(group, permission, world); - return true; + return holderRemovePermission(group, permission, world); } // utility methods for getting user and group instances @@ -380,7 +365,7 @@ public class VaultPermissionHook extends AbstractVaultPermission { // utility methods for modifying the state of PermissionHolders - private void holderAddPermission(PermissionHolder holder, String permission, String world) { + private boolean holderAddPermission(PermissionHolder holder, String permission, String world) { Objects.requireNonNull(permission, "permission is null"); Preconditions.checkArgument(!permission.isEmpty(), "permission is an empty string"); @@ -388,14 +373,13 @@ public class VaultPermissionHook extends AbstractVaultPermission { logMsg("#holderAddPermission: %s - %s - %s", holder.getFriendlyName(), permission, world); } - this.executor.execute(() -> { - if (holder.setPermission(NodeFactory.make(permission, true, getVaultServer(), world)).asBoolean()) { - holderSave(holder); - } - }); + if (holder.setPermission(NodeFactory.make(permission, true, getVaultServer(), world)).asBoolean()) { + return holderSave(holder); + } + return false; } - private void holderRemovePermission(PermissionHolder holder, String permission, String world) { + private boolean holderRemovePermission(PermissionHolder holder, String permission, String world) { Objects.requireNonNull(permission, "permission is null"); Preconditions.checkArgument(!permission.isEmpty(), "permission is an empty string"); @@ -403,22 +387,32 @@ public class VaultPermissionHook extends AbstractVaultPermission { logMsg("#holderRemovePermission: %s - %s - %s", holder.getFriendlyName(), permission, world); } - this.executor.execute(() -> { - if (holder.unsetPermission(NodeFactory.make(permission, getVaultServer(), world)).asBoolean()) { - holderSave(holder); - } - }); + if (holder.unsetPermission(NodeFactory.make(permission, getVaultServer(), world)).asBoolean()) { + return holderSave(holder); + } + return false; } - void holderSave(PermissionHolder holder) { + boolean holderSave(PermissionHolder holder) { if (holder.getType().isUser()) { User u = (User) holder; + + // we don't need to join this call - the save operation + // can happen in the background. this.plugin.getStorage().saveUser(u); - } - if (holder.getType().isGroup()) { + } else if (holder.getType().isGroup()) { Group g = (Group) holder; - this.plugin.getStorage().saveGroup(g).thenRunAsync(() -> this.plugin.getUpdateTaskBuffer().request(), this.plugin.getBootstrap().getScheduler().async()); + + // invalidate caches - they have potentially been affected by + // this change. + this.plugin.getGroupManager().invalidateAllGroupCaches(); + this.plugin.getUserManager().invalidateAllUserCaches(); + + // we don't need to join this call - the save operation + // can happen in the background. + this.plugin.getStorage().saveGroup(g); } + return true; } // helper methods to just pull values from the config. diff --git a/common/src/main/java/me/lucko/luckperms/common/command/utils/StorageAssistant.java b/common/src/main/java/me/lucko/luckperms/common/command/utils/StorageAssistant.java index 18656311..d2690bdf 100644 --- a/common/src/main/java/me/lucko/luckperms/common/command/utils/StorageAssistant.java +++ b/common/src/main/java/me/lucko/luckperms/common/command/utils/StorageAssistant.java @@ -130,7 +130,8 @@ public final class StorageAssistant { return; } - plugin.getUpdateTaskBuffer().requestDirectly(); + plugin.getGroupManager().invalidateAllGroupCaches(); + plugin.getUserManager().invalidateAllUserCaches(); Optional messagingService = plugin.getMessagingService(); if (messagingService.isPresent() && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { @@ -154,7 +155,8 @@ public final class StorageAssistant { return; } - plugin.getUpdateTaskBuffer().requestDirectly(); + plugin.getGroupManager().invalidateAllGroupCaches(); + plugin.getUserManager().invalidateAllUserCaches(); Optional messagingService = plugin.getMessagingService(); if (messagingService.isPresent() && plugin.getConfiguration().get(ConfigKeys.AUTO_PUSH_UPDATES)) { diff --git a/common/src/main/java/me/lucko/luckperms/common/managers/group/AbstractGroupManager.java b/common/src/main/java/me/lucko/luckperms/common/managers/group/AbstractGroupManager.java index 0ae70a6f..32b695f3 100644 --- a/common/src/main/java/me/lucko/luckperms/common/managers/group/AbstractGroupManager.java +++ b/common/src/main/java/me/lucko/luckperms/common/managers/group/AbstractGroupManager.java @@ -27,6 +27,7 @@ package me.lucko.luckperms.common.managers.group; import me.lucko.luckperms.common.managers.AbstractManager; import me.lucko.luckperms.common.model.Group; +import me.lucko.luckperms.common.model.PermissionHolder; import java.util.Optional; @@ -63,4 +64,9 @@ public abstract class AbstractGroupManager extends AbstractMana protected String sanitizeIdentifier(String s) { return s.toLowerCase(); } + + @Override + public void invalidateAllGroupCaches() { + getAll().values().forEach(PermissionHolder::invalidateCachedData); + } } \ No newline at end of file diff --git a/common/src/main/java/me/lucko/luckperms/common/managers/group/GroupManager.java b/common/src/main/java/me/lucko/luckperms/common/managers/group/GroupManager.java index fb4ef00d..8c6f01c4 100644 --- a/common/src/main/java/me/lucko/luckperms/common/managers/group/GroupManager.java +++ b/common/src/main/java/me/lucko/luckperms/common/managers/group/GroupManager.java @@ -38,4 +38,9 @@ public interface GroupManager extends Manager */ T getByDisplayName(String name); + /** + * Invalidates the cached data for *loaded* groups. + */ + void invalidateAllGroupCaches(); + } diff --git a/common/src/main/java/me/lucko/luckperms/common/managers/user/AbstractUserManager.java b/common/src/main/java/me/lucko/luckperms/common/managers/user/AbstractUserManager.java index 566afd81..303109f4 100644 --- a/common/src/main/java/me/lucko/luckperms/common/managers/user/AbstractUserManager.java +++ b/common/src/main/java/me/lucko/luckperms/common/managers/user/AbstractUserManager.java @@ -29,6 +29,7 @@ import me.lucko.luckperms.api.Node; import me.lucko.luckperms.api.context.ImmutableContextSet; import me.lucko.luckperms.common.config.ConfigKeys; import me.lucko.luckperms.common.managers.AbstractManager; +import me.lucko.luckperms.common.model.PermissionHolder; import me.lucko.luckperms.common.model.User; import me.lucko.luckperms.common.model.UserIdentifier; import me.lucko.luckperms.common.node.factory.NodeFactory; @@ -153,6 +154,11 @@ public abstract class AbstractUserManager extends AbstractManage ); } + @Override + public void invalidateAllUserCaches() { + getAll().values().forEach(PermissionHolder::invalidateCachedData); + } + /** * Check whether the user's state indicates that they should be persisted to storage. * diff --git a/common/src/main/java/me/lucko/luckperms/common/managers/user/UserManager.java b/common/src/main/java/me/lucko/luckperms/common/managers/user/UserManager.java index b95e0d5d..47ca755b 100644 --- a/common/src/main/java/me/lucko/luckperms/common/managers/user/UserManager.java +++ b/common/src/main/java/me/lucko/luckperms/common/managers/user/UserManager.java @@ -80,8 +80,13 @@ public interface UserManager extends Manager updateAllUsers(); + /** + * Invalidates the cached data for *loaded* users. + */ + void invalidateAllUserCaches(); + } diff --git a/common/src/main/java/me/lucko/luckperms/common/tasks/ExpireTemporaryTask.java b/common/src/main/java/me/lucko/luckperms/common/tasks/ExpireTemporaryTask.java index 90a42de0..995f277f 100644 --- a/common/src/main/java/me/lucko/luckperms/common/tasks/ExpireTemporaryTask.java +++ b/common/src/main/java/me/lucko/luckperms/common/tasks/ExpireTemporaryTask.java @@ -60,7 +60,8 @@ public class ExpireTemporaryTask implements Runnable { } if (groupChanges) { - this.plugin.getUpdateTaskBuffer().request(); + this.plugin.getGroupManager().invalidateAllGroupCaches(); + this.plugin.getUserManager().invalidateAllUserCaches(); } } diff --git a/common/src/main/java/me/lucko/luckperms/common/tasks/UpdateTask.java b/common/src/main/java/me/lucko/luckperms/common/tasks/UpdateTask.java index db8f35c6..f3a4c623 100644 --- a/common/src/main/java/me/lucko/luckperms/common/tasks/UpdateTask.java +++ b/common/src/main/java/me/lucko/luckperms/common/tasks/UpdateTask.java @@ -70,6 +70,7 @@ public class UpdateTask implements Runnable { this.plugin.getStorage().loadAllTracks().join(); // Refresh all online users. + this.plugin.getUserManager().invalidateAllUserCaches(); CompletableFuture userUpdateFut = this.plugin.getUserManager().updateAllUsers(); if (!this.initialUpdate) { userUpdateFut.join(); diff --git a/sponge/src/main/java/me/lucko/luckperms/sponge/service/internal/HolderSubjectData.java b/sponge/src/main/java/me/lucko/luckperms/sponge/service/internal/HolderSubjectData.java index 56c12221..7ce0c94e 100644 --- a/sponge/src/main/java/me/lucko/luckperms/sponge/service/internal/HolderSubjectData.java +++ b/sponge/src/main/java/me/lucko/luckperms/sponge/service/internal/HolderSubjectData.java @@ -451,7 +451,9 @@ public class HolderSubjectData implements LPSubjectData { if (this.type == NodeMapType.TRANSIENT) { // don't bother saving to primary storage. just refresh if (t.getType().isGroup()) { - return this.service.getPlugin().getUpdateTaskBuffer().request(); + this.service.getPlugin().getGroupManager().invalidateAllGroupCaches(); + this.service.getPlugin().getUserManager().invalidateAllUserCaches(); + return CompletableFuture.completedFuture(null); } } @@ -461,8 +463,9 @@ public class HolderSubjectData implements LPSubjectData { return this.service.getPlugin().getStorage().saveUser(user); } else { Group group = ((Group) t); - return this.service.getPlugin().getStorage().saveGroup(group) - .thenCompose(v -> this.service.getPlugin().getUpdateTaskBuffer().request()); + this.service.getPlugin().getGroupManager().invalidateAllGroupCaches(); + this.service.getPlugin().getUserManager().invalidateAllUserCaches(); + return this.service.getPlugin().getStorage().saveGroup(group); } } }