From 4d91dd604d780ee75f3b02958241a76b1e3934fb Mon Sep 17 00:00:00 2001 From: MrPowerGamerBR Date: Tue, 21 Nov 2023 21:09:22 -0300 Subject: [PATCH] Improve EntityScheduler optimization patch --- README.md | 4 +- ...uler-s-executeTick-checks-if-there-i.patch | 41 ++++++------------- 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index a05173e..996a4e5 100644 --- a/README.md +++ b/README.md @@ -39,8 +39,8 @@ SparklyPaper's config file is `sparklypaper.yml`, the file is, by default, place * Optimize `EntityScheduler`'s `executeTick` * On each tick, Paper runs `EntityScheduler`'s `executeTick` of each entity. This is a bit expensive, due to `ArrayDeque`'s `size()` call because it ain't a simple "get the current queue size" function, and due to the thread checks. * To avoid those hefty calls, we check if we *really* need to execute the `executeTick`, by doing two changes: - * First, we check if the scheduler had a task scheduled to it at least once, if not, we bail out early and quickly. In this case, we don't even increase the scheduler's `tickCount`, nor do we check if the scheduler has been retired. This is not an issue, since `tickCount` is relative and not bound to the server's current tick, so it doesn't matter if we don't increase it. - * Then, we check if the scheduler has any pending tasks by checking if `currentlyExecuting` is not empty or if `oneTimeDelayed` is not empty. This avoids the thread checks and unnecessary `size()` calls. + * First, we check if the scheduler actually has scheduled tasks to it by looking up `hasScheduledTasks`, if not, we bail out early and quickly. In this case, we don't even increase the scheduler's `tickCount`, nor do we check if the scheduler has been retired. This is not an issue, since `tickCount` is relative and not bound to the server's current tick, so it doesn't matter if we don't increase it. + * Then, we check if the scheduler has any pending tasks by checking if `currentlyExecuting` is not empty or if `oneTimeDelayed` is not empty. If it isn't, then we process the tasks, if it is, then we set `hasScheduledTasks` to false, avoiding future `size()` checks. This avoids the thread checks and unnecessary `size()` calls. * Most entities won't have any scheduled tasks, so this is a nice performance bonus. These optimizations, however, wouldn't work in a Folia environment, but because in SparklyPaper `executeTick` is always executed on the main thread, it ain't an issue for us (yay). With this change, the `executeTick` loop in `tickChildren` CPU % usage drops from 2.48% to 0.61% in a server with ~12k entities! * Of course, this doesn't mean that `ArrayDeque#size()` is slow! It is mostly that because the `executeTick` function is called each tick for each entity, it would be better for us to avoid as many useless calls as possible. * Blazingly Simple Farm Checks diff --git a/patches/server/0010-Skip-EntityScheduler-s-executeTick-checks-if-there-i.patch b/patches/server/0010-Skip-EntityScheduler-s-executeTick-checks-if-there-i.patch index 679e704..a4fc707 100644 --- a/patches/server/0010-Skip-EntityScheduler-s-executeTick-checks-if-there-i.patch +++ b/patches/server/0010-Skip-EntityScheduler-s-executeTick-checks-if-there-i.patch @@ -11,14 +11,14 @@ To avoid the hefty ArrayDeque's size() call, we check if we *really* need to exe Most entities won't have any scheduled tasks, so this is a nice performance bonus. These optimizations, however, wouldn't work in a Folia environment, but because in SparklyPaper executeTick is always executed on the main thread, it ain't an issue for us (yay). diff --git a/src/main/java/io/papermc/paper/threadedregions/EntityScheduler.java b/src/main/java/io/papermc/paper/threadedregions/EntityScheduler.java -index 62484ebf4550b05182f693a3180bbac5d5fd906d..9e15a2a9e47a5b50c3c9e2afce2eb0f48ed0fdaf 100644 +index 62484ebf4550b05182f693a3180bbac5d5fd906d..4af6652edc0651ab8ad28a738055d407451e49bf 100644 --- a/src/main/java/io/papermc/paper/threadedregions/EntityScheduler.java +++ b/src/main/java/io/papermc/paper/threadedregions/EntityScheduler.java @@ -45,6 +45,7 @@ public final class EntityScheduler { private final Long2ObjectOpenHashMap> oneTimeDelayed = new Long2ObjectOpenHashMap<>(); private final ArrayDeque currentlyExecuting = new ArrayDeque<>(); -+ private boolean hasScheduledAtLeastOneTask = false; // SparklyPaper - skip EntityScheduler's executeTick checks if there isn't any tasks to be run ++ private boolean hasScheduledTasks = false; // SparklyPaper - skip EntityScheduler's executeTick checks if there isn't any tasks to be run public EntityScheduler(final CraftEntity entity) { this.entity = Validate.notNull(entity); @@ -26,11 +26,11 @@ index 62484ebf4550b05182f693a3180bbac5d5fd906d..9e15a2a9e47a5b50c3c9e2afce2eb0f4 if (this.tickCount == RETIRED_TICK_COUNT) { return false; } -+ hasScheduledAtLeastOneTask = true; // SparklyPaper - skip EntityScheduler's executeTick checks if there isn't any tasks to be run ++ hasScheduledTasks = true; // SparklyPaper - skip EntityScheduler's executeTick checks if there isn't any tasks to be run this.oneTimeDelayed.computeIfAbsent(this.tickCount + Math.max(1L, delay), (final long keyInMap) -> { return new ArrayList<>(); }).add(task); -@@ -138,15 +140,34 @@ public final class EntityScheduler { +@@ -138,6 +140,23 @@ public final class EntityScheduler { * @throws IllegalStateException If the scheduler is retired. */ public void executeTick() { @@ -39,33 +39,18 @@ index 62484ebf4550b05182f693a3180bbac5d5fd906d..9e15a2a9e47a5b50c3c9e2afce2eb0f4 + // main thread, we don't need to care about concurrency + // First, we use a simple boolean check here + // Yeah, this sounds dumb but, after benchmarking, it seems like a simple boolean check like this is actually more performant than calling both is empty checks -+ // So first we check if the EntityScheduler has been used at least ONCE -+ // If it wasn't, then we skip all checks entirely -+ // We don't even need to update the tick count because the tick count is actually relative to this scheduler, not bound to the global current tick, so it isn't a big deal if we don't update the tick count -+ if (!this.hasScheduledAtLeastOneTask) ++ // So first we check if the EntityScheduler has any scheduled tasks ++ // If it doesn't, then we skip all checks entirely ++ // We don't even need to update the tick count because the tick count is actually relative to this scheduler, not bound to the global current tick, so it isn't a big deal if we don't update the tick count, as long ++ // as there isn't any pending tasks ++ if (!this.hasScheduledTasks) ++ return; ++ // We do have scheduled tasks then... but do we *really* have scheduled tasks tho? ++ if (this.currentlyExecuting.isEmpty() && this.oneTimeDelayed.isEmpty()) { // Check if we have any pending tasks and, if not, skip! ++ this.hasScheduledTasks = false; // We don't! Bye bye!! + return; -+ if (this.tickCount == RETIRED_TICK_COUNT) { -+ throw new IllegalStateException("Ticking retired scheduler"); + } -+ ++this.tickCount; -+ if (this.currentlyExecuting.isEmpty() && this.oneTimeDelayed.isEmpty()) // Check if we have any pending tasks and, if not, skip! -+ return; + // SparklyPaper end final Entity thisEntity = this.entity.getHandleRaw(); TickThread.ensureTickThread(thisEntity, "May not tick entity scheduler asynchronously"); - final List toRun; - synchronized (this.stateLock) { -- if (this.tickCount == RETIRED_TICK_COUNT) { -- throw new IllegalStateException("Ticking retired scheduler"); -- } -- ++this.tickCount; -+ // SparklyPaper start - skip EntityScheduler's executeTick checks if there isn't any tasks to be run -+ // if (this.tickCount == RETIRED_TICK_COUNT) { -+ // throw new IllegalStateException("Ticking retired scheduler"); -+ // } -+ // ++this.tickCount; -+ // SparklyPaper end - if (this.oneTimeDelayed.isEmpty()) { - toRun = null; - } else {