Fast path for self-triggered IC thinks - #1379
Conversation
Every self-triggered IC paid for a full sign snapshot (getState), component serialization of all four lines and an IC id regex match on every think tick, even though the IC instance itself is cached. At 2000 STs that measured +3.4ms per tick with spikes to 100ms. The think handler now reuses the cached IC and family directly and only reruns the full setupIC verification once a second per IC; entries are only honoured while the IC remains in ICManager's cache, so break/unload invalidation is unchanged, and a sign edit is picked up within a second. Chunk-loaded lookups in the ST sweep are also memoised per pass, since clustered STs ask about the same few chunks thousands of times and a chunk cannot load or unload mid-sweep. Re-measured: 2000 active STs are indistinguishable from an idle server (50.5ms avg tick vs 50.3 baseline).
| Map<World, Map<Long, Boolean>> loadedCache = new HashMap<>(); | ||
| for (Location location : registeredLocations) { | ||
| if(!location.getWorld().isChunkLoaded(location.getBlockX() >> 4, location.getBlockZ() >> 4)) { | ||
| if(!isLoadedCached(location.getWorld(), location.getBlockX() >> 4, location.getBlockZ() >> 4, loadedCache)) { |
There was a problem hiding this comment.
What's the actual cost of checking if a chunk is loaded now? Last I checked this was functionally equivalent to a complex map lookup, which I would assume this would basically mirror.
There was a problem hiding this comment.
Fair point. The measured win came from skipping the sign snapshot and the id regex, the memoisation was added on top and never measured on its own, and with isChunkLoaded being a cheap map lookup it just traded one lookup for two plus boxing. Dropped it, the checks call isChunkLoaded directly again.
isChunkLoaded is already a cheap map lookup on modern servers, so the cache traded one lookup for two plus boxing. The measured win came from skipping the sign snapshot and regex, which stays.
|
|
||
| if(icData != null && icData[2] instanceof SelfTriggeredIC ic) { | ||
| event.setHandled(true); | ||
| if (thinkFastCache.size() > MAX_THINK_FAST_ENTRIES) |
There was a problem hiding this comment.
This here feels weird to me, it fully clears the entire cache if it hits the limit? That feels like it'd create a tonne of churn?
There was a problem hiding this comment.
CraftBook's HistoryHashMap might end up working better here rather than manually performing this validation, it has a maxEntries option
Otherwise one of the Guava cache classes, as that'd also allow you to drop the time revalidation measure too
There was a problem hiding this comment.
Yeah, fair - it dumps all 4096 to make room for one, and anything sitting above the limit would keep landing back on the slow path in waves. Which is the cost this is meant to avoid. Fixed.
There was a problem hiding this comment.
Went with Guava. expireAfterWrite replaces the deadline I was storing, so the value is just the ICFamily now instead of an Object[] with a timestamp to unbox. Same shape as ItemSyntax and ParsingUtil already use.
| if(!EventUtil.passesFilter(event)) return; | ||
|
|
||
| // Fast path: a cached self-triggered IC thinks without re-snapshotting and | ||
| // re-parsing its sign (getState + component serialization + regex, per IC |
There was a problem hiding this comment.
This comment feels a bit bulky – It should probably just say it's a cached fast path, the rest doesn't really add much
There was a problem hiding this comment.
Trimmed to one line.
Review feedback: clearing the whole map at the limit churns every entry to make room for one, and on a server above the limit that puts every IC back on the slow path in waves - the cost the fast path exists to avoid. CacheBuilder with maximumSize evicts one entry instead, and expireAfterWrite replaces the hand-rolled deadline, so the value is just the ICFamily rather than an Object[] with a timestamp to unbox and compare. Guava caches are already used this way in ItemSyntax and ParsingUtil. Comment trimmed to what it is.
2 participants
Every self-triggered IC pays for a full sign snapshot (getState), component serialization of all four lines and an IC id regex match on every think tick, even though the IC instance itself is already cached. At 2000 STs that measured +3.4ms per tick with spikes to 100ms.
The think handler now reuses the cached IC and family directly, and the full setupIC verification still reruns once a second per IC. The fast entry is only honoured while the IC remains in ICManager's cache, so break/unload invalidation is unchanged, and a sign edit is picked up within a second. The think itself runs exactly the code it always did (same IC instance, same sign object), so variables etc are untouched.
Chunk-loaded lookups in the ST sweep are also memoised per pass: clustered STs ask about the same few chunks thousands of times in one sweep, and a chunk cannot load or unload mid-sweep.
Re-measured after the change: 2000 active STs are indistinguishable from an idle server (50.5ms avg tick vs 50.3 baseline).