EngineHub
Pipes: performance improvements and some correctness fixes
Overview
This PR dramatically improves the performance of Pipes, fixes some correctness and, for the very first time, allows users to simply live out their crazy dreams of ginormous warehouses - some of our players have 20k+ glass-blocks and thousands of output-pistons, spanning many (unloaded) chunks, which we support without breaking a sweat.
I am well aware that a refactor like this is not easy to adopt. Seeing how I just recently ported my experiments into this more structured and stepwise implementation, I would still call it experimental. I need as much constructive feedback and actual field-testing as I can get, such that we can make these performance-improvements become available on the large scale.
This PR may look like it affects many sites within the project, but it really doesn't - the changes I had to make to files besides Pipes.java are tiny; I mostly added files and rewrote the mechanic itself. I'd call it rather self-contained.
Details
At first, I gave my best to incrementally improve maintainability by untangling large methods and reducing dead- as well as copy-pasted code; I also took the liberty of renaming and unifying cryptic/inconsistent variable-names, since they stood in the way of me fully understanding the codebase at hand.
I fixed Jukeboxes; see https://github.com/EngineHub/CraftBook/issues/1361 and https://github.com/EngineHub/CraftBook/issues/1289. While I was at it, I also added proper support for Brewing-Stands, seeing how they were always included via InventoryUtil#doesBlockHaveInventory when putting, but excluded when sucking; may come in handy for some users.
I introduced the PipeSign, which wraps include- and exclude-filters as well as the logic for parsing them from a sign - this class also makes filters easily cacheable.
Having profiled the mechanic a bunch, I noticed how much needless computational overhead the usage of sets in filters add, due to how they are scattered all over the heap and thus harder to iterate, so I migrated to lists; there's no real-world advantage to sets anyway.
The first important change was to use a LongOpenHashSet for keeping track of visited blocks while enumerating. Calling Block#getLocation() and Location#toVector causes countless allocations. I introduced the concept of compact-ids, which are merely bit-packed longs containing coordinates as well as, where needed, a unique, incrementing world-id.
Next up, I introduced a cache for all blocks the pipe needs to access, called CachedBlock; it stores: tube-color (if any), is-valid-pipe-block, has-inventory, is-sign, piston-facing (if any) as well as the material itself. Colors are now described by TubeColor, which is a much more readable version of the gigantic switch we used prior - I also added support for tinted glass, for uniformity, such that we really do support all glass variants (even if it has no pane-version (yet)).
The BlockCache handles proper invalidation and also keeps loaded chunks retained in memory, for a configurable amount of time, which is the only way to make long pipes work efficiently, as to avoid fatal unload/load-cycles which will tank TPS. If on Paper, chunks will be loaded asynchronously. There's never more than a single chunk-load per tick, as to distribute load evenly, and there are "cache-miss"-counters in place, paired with configurable per-tick limits. Blocks that are only needed once (glass, pistons, signs, etc.) are retained for shorter periods than those which will be accessed regularly when putting (containers).
Having cached blocks be mere integers saves a ton of memory and once again avoids allocations. We must not forget how many, in some cases, hundreds of thousands there will be, due to the nature of enumerating all faces of the current pipe-block while walking, as well as the various faces of pistons where signs could be mounted on.
Once the cache has been warmed up for a given pipe, the system will no longer need to access the world for traversal/filtering - only to suck/put items. This makes all the difference in the world, seeing how painfully slow Bukkit APIs are, due to their immutable and uncached nature.
Signs and thereby filters are cached just as well, with the key being a compact-id of their mounting-block (piston) for fast resolving. When creating cached signs, the PipeSignCacheCreatedEvent is fired and when invalidating them again (changed, destroyed, etc.), the PipeSignCacheInvalidedEvent is fired; this allows external plugins who seek to modify the filtering-process to keep their own separate, independent caches. A plugin of mine uses them to read data from the sign's PDC and cache the parsed result, to be later used with the PipeFilterEvent.
All internal listeners that react to pipe-events have been modified to use a fast-path when checking if the putting-block is a sign, seeing how that's the overwhelming minority of cases and the code that would've been executed otherwise noticeably weighed down the mechanic.
Since users who have really long pipes may become confused due to initial perceived inactivity when warming up, I've implemented "warmup-notifications": simple actionbar-messages that are sent to players within a certain configurable radius (or -1 to disable it) of the input-block.
Thanks for this- I haven't had a chance to fully look over the code, but it seems to vaguely resemble my plans for CraftBook 5's pipes- (computing pipes and then storing locations in some kind of spatial tree, allowing tree accesses to invalidate the pipe if it's possibly modified- with optional bloom filter if found necessary for performance reasons). Overall I do feel an overhaul this large makes sense to put into CraftBook 5, rather than CB3.
From a quick look though, this current implementation feels like it makes a few assumptions that are not really safe to do for general cases. Eg, there are surprisingly servers with hundreds of worlds that have active pipes in them, so an assumption that only 8 exists isn't really feasible- any limit doesn't work. I also am mildly confused about how some of the current systems (such as TubeColor) might impact long-term flexibility (or possibly mod support, in the case of hybrid servers).
I feel that for CB3, some parts that can be fairly easily split out, such as bugfixes/etc would definitely make sense- however a full overhaul to the foundations of how it works probably would need to happen in CB5. Especially as existing Pipes might have been made based on the quirks in the ordering/etc that the system currently has- so changing that system within CB3 is something that we'd try to avoid.
Personally, I wouldn't try to get too smart when it comes to how one caches pipe-structures, because the gains in performance are in no sane ratio to the complexity of invalidating that cache properly. The way I currently do it, with simply layering a cache on top of the block-accesses the enumeration-algorithm itself makes, seems like a good balance. Walking the pipe isn't really an issue - getting block-types, signs, facings, etc. over and over again is, not to mention loading chunks synchronously, just so that they become unloaded again during the next few ticks, which is deadly in pipes that periodically collect from, say, farms with magnet-chests. As an example: one of our users has a pipe that spans just over 32k pipe-blocks with around 6k output-pistons in total and walking the whole thing feels basically instant, once the cache is warmed up. The cache also only consumes a hand full of megabytes.
That said, I'd love to see another approach come to life - because having choice is always an advantage!
Oh wow, I did not know how many worlds some servers have! You are very right in saying that I made assumptions - I have to, because otherwise, I would make zero progress and get stuck in action-paralysis. We needed a solution now. That said, removing worlds from compact-ids entirely, in favor of a cache-instance per world, would be no issue whatsoever. Actually, it might even speed things up, seeing how there are less hashing-collisions on servers that do use pipes in multiple worlds in parallel. Great food for thoughts!
I don't see how the concept of TubeColor impacts flexibility - quite the opposite, in my opinion. Currently, CraftBook utilizes a mapping from Material to DyeColor - in essence, that's the same thing. How is that more supporting of hybrid servers? In general, since you're worrying about my version behaving differently, it'd be really helpful if you could name the exact divergences such that I can consider taking care of them.
It's less about specific divergences, and moreso that this is a very large-scale rewrite on a legacy branch of a plugin that's in maintenance mode, and mostly kept around for legacy compatibility.
It doesn't make sense to do such a major change that could potentially alter behaviour (even unintentionally), on a version of a plugin that exists specifically to keep existing behaviour.
The main reason I suggest this for CraftBook 5 instead, is that it's not held to the same legacy requirements. As mentioned in the previous comment, if there are specific bug fixes etc that could still be split out & applied to CraftBook 3 that'd make sense, but a full overhaul would need to be targeted for CraftBook 5. It's not about identifiable divergences in behaviour, but moreso that any change this large has the potential to cause divergences in behaviour, when that's specifically what CB5 is made for
I see! I'm glad to respect your policy regarding the stability of CraftBook 3. If you ever do consider reworking the mechanic for a possible reintroduction in CraftBook 5 and find a way to provide similar or maybe even better performance than my patch does, I'd be up to help with the implementation wherever possible.