[1.12.2] Optimize wire system powered checks#4700
Conversation
this avoids collecting the intermediate result from WorldSavedDataWireSystems#getWireSystemsWithElement(WireSystem.WireElement) into a List just to get a stream over the same list again, and instead evaluates the entire expression directly. that said, WorldSavedDataWireSystems#getWireSystemsWithElement(WireSystem.WireElement) should still be able to be optimized a lot further (it's currently O(N), lol)
this makes WireSystem#hashCode() cache its return value, which previously accounted for most of WireManager#isPowered's CPU time. ...and this can still be optimized even further to avoid iterating over every wire system in the world on every operation!
this will make it much safer to implement the next optimizations to WorldSavedDataWireSystems#getWireSystemsWithElement(WireSystem.WireElement)
we now use an index in WorldSavedDataWireSystems to keep track of which WireSystem each WireSystem.WireElement belongs to, which allows us to avoid iterating over every element of every wire system in the world for every pipe gate every tick.
…ity) instead of explicitly marking the chunk as dirty this will make it play nicer with cubic chunks
| if (chunk != null) { | ||
| chunk.markDirty(); | ||
| } | ||
| world.markChunkDirty(this.pos, this); |
There was a problem hiding this comment.
Funnily enough this itself was an optimization - going through our cache was cheaper then calling BlockEntity.markDirty. I'm not sure if I tried using World.markChunkDirty though - since it does less work.
There was a problem hiding this comment.
Yeah, I guessed that was the point, however I figured it wasn't much of a performance concern as it wasn't showing up in profiling results at all, and the vanilla version of World.markChunkDirty isn't too much different than the existing one (hashtable lookup through ChunkProviderServer instead of accessing the cached chunk instance, but presumably negligible impact if I already couldn't measure it before).
However in Cubic Chunks worlds World.markChunkDirty actually marks the cube as dirty, which is important because otherwise the cube might not get saved. (I actually saw this happen in practice - a BuildCraft quarry in a Cubic Chunks world piping items into a crate from another mod which was also not using World.markChunkDirty resulted in the mined blocks not being saved, even though the blocks broken by the quarry were!)
If you're not certain about this change I can totally move it into a separate PR. I just threw it in here since I figured it wouldn't make too much difference, but wouldn't mind profiling it in more depth if you'd like.
There was a problem hiding this comment.
Back in my day, what we would do is put certain optimizations behind compatibility flags (f.e. we had an important optimization in pipes that broke on Cauldron-based servers). Maybe this would be the way to go here?
|
Thanks for looking into this! I haven't actually tested it yet but it all looks good. |
|
Sorry for the delay - this works great, and I'll push it out soon |
3 participants
This PR optimizes
WireManager#isPowered(EnumWirePart)and methods used by it in the following ways:WireSystem#hashCode()is pre-computed and stored in a fieldHashMaplookups when accessingWorldSavedDataWireSystems#wireSystemssignificantly faster (HashMap#get(Object)previously took around 30% ofWireManager#isPowered(EnumWirePart)'s total CPU time according to my profiling, and now doesn't even get picked up by VisualVM any more)Map<WireSystem.WireElement, List<WireSystem>>toWorldSavedDataWireSystemswhich makes it possible to implementWorldSavedDataWireSystems#getWireSystemsWithElement(WireSystem.WireElement)efficientlyWireSystem.WireElementin everyWireSystemon every call toWorldSavedDataWireSystems#getWireSystemsWithElement(WireSystem.WireElement), which doesn't scale very well (especially when you consider that it gets called once per tick, per gate trigger, per pipe wire attached to the same pipe as the gate)WireManager/WorldSavedDataWireSystemswith the fully written-out "normal" code, as the very deep call chains caused by using streams were badly limiting the JVM's ability to inline things, and also filling up the heap very quickly with a lot of garbage (no inlining -> no escape analysis -> objects actually land on the heap -> incalculable pain and suffering)WireSystemimmutable, as modifying it would have affected the hash code changing (would have been annoying to deal with invalidating the cached hash code), not to mention that allowing it to be modified kinda conflicts with its primary role as aHashMapkey inWorldSavedDataWireSystems. IMO this is much cleaner and will certainly prevent anyone from trying to use it incorrectly, however the rest of the optimizations can still function without this change if necessary.Extra bonus fixes and improvements:
Map#replaceAll(BiFunction)inWorldSavedDataWireSystems#tick()to avoid a pointlessputfor everyWireSystemevery time a gate changesWorld#markChunkDirty(BlockPos, TileEntity)inTileBC_Neptune#markChunkDirty()rather than explicitly getting the chunk and callingmarkDirty(), this ensures the tile entity actually gets saved when using Cubic Chunks is being usedAll together, this reduced the total CPU time spent in
WireManager#isPowered(EnumWirePart)by as much as 200x (times, not percent!) on one of my larger worlds, shaving off a solid 30ms per tick.Not sure about code style, let me know if I should change anything :)