TechReborn icon indicating copy to clipboard operation
TechReborn copied to clipboard

Spread out slot transfer actions & caching

Open aria1th opened this issue 2 years ago • 9 comments

!! It requires reviews I was using this for months so it should be okay tho...

Changes : Transfer Behavior : its transfer speed is actually equal, but it does not instantly transfers items. -Now it inputs items then outputs items in separate tick, so it efficiently spreads out mspt.

Caches InventoryStorage.of
    -Should not affect anything and just 2% performance boost
Prevent SyncWithAll being spammed
    -reduces packets... but seems there's more packet spam issues, it generates thousands of packets...

Performance boosts: Tested with 670 storage units working constantly, reduced 4.6mspt -> 2.67mspt

aria1th avatar Apr 09 '22 07:04 aria1th

Small idea, but can we spread out packet sync timing by using lastTickedSync = world.random.nextInt(20); ?

aria1th avatar Apr 09 '22 12:04 aria1th

-Now it inputs items then outputs items in separate tick, so it efficiently spreads out mspt.

"Spreading out" doesn't make sense imo, it still uses the same amount of time in total.

Performance boosts: Tested with 670 storage units working constantly, reduced 4.6mspt -> 2.67mspt

Why? Is it just caching the storages? I see 3 somewhat unrelated changes, you should only keep the one(s) that actually make(s) a difference.

Technici4n avatar Apr 21 '22 17:04 Technici4n

Actual difference came from spreading out its working timing, which was not ready for review. Storages or machines being loaded at once shared its working timing and intervals, it had bad performance and packet issue. Especially industrial chunk loader-loaded machines will share its timing, on world load....

I randomized its starting point and actual difference was made. Caching itself was very important too, however.

Sync timing limit was not helpful at all, since storages were not spamming unlike machines, so I should revert it.

aria1th avatar Apr 21 '22 18:04 aria1th

I really don't see why spreading out work helps? It reduces tps variance, but shouldn't change mean tps?

Technici4n avatar Apr 21 '22 18:04 Technici4n

hmm yeah, I inspected more and found that 'instant input-output' is now disabled, not sure if its intended. Storage or general machines are having 2 ticks to handle items, instead of outputting instantly when its auto input + auto output. I tested with very intense bottleneck condition with constant flow of 30+ items IO is happening, and its difference is mostly from less bottleneck and more item merging.

aria1th avatar Apr 21 '22 18:04 aria1th

so are you saying we don't need this PR after all?

Ayutac avatar Apr 21 '22 19:04 Ayutac

Not really, this partially solves some weird issues with highest mspt being observed as 200mspt, and internet connection issues with packet limit conditions when 400+ storage units are doing constant I/O (3200+ packets per seconds per player condition). Test case is very extreme, so I'd say it helped mspt more from having more chance to merge items rather than others : ezgif-4-d57c18b364

aria1th avatar Apr 21 '22 19:04 aria1th

I... would rather have these extreme cases deal with that themselves before we accidentically introduce bugs that affect 95% of the playerbase instead of the 5% it might be actually be made for. But that is just my POV

Ayutac avatar Apr 21 '22 19:04 Ayutac

I couldn't see any bugs or serious effects with this, as I'm already using it in my server... even before this, storage units were not instant dropper and its update order was not promising. So it shouldn't really affect something, except for some really extreme cases :

Case A - someone was using 4-overclocked electric furnace as 'pipe', so they used input slot for Auto Input + Auto Output General Case : someone was using 1-2 tick recipe crafter as 'pipe', then items will be processed before being outputted

but still it requires review, to prevent any unknown bugs or issues.. should I separate PR, one with separate actions / one with caching inventories?

aria1th avatar Apr 21 '22 19:04 aria1th