MCHPR / MCHPRS

A multithreaded Minecraft server built for redstone.
MIT License
1.55k stars 68 forks source link

Duplicate signal strength to all outputs #129

Closed BramOtte closed 6 months ago

BramOtte commented 8 months ago

We duplicate the signal strength of a component to all outputs, so we can get the input signal strength without having to fetch the inputs each time. Additionally by having separate buckets for each signal strength the combined input strength is calculated with just a few u128 instructions instead of having to loop over all the input. This results in the first tenth of the chungus benchmark to only take ~7.6 on my machine while without this change it takes ~9.9 seconds, so ~30% faster. Tested on: Ubuntu 22.04.3 LTS, intel core I7-10750H

Paul1365972 commented 7 months ago

Wow wouldn't have thought that such huge performance improvements were still possible. Confirmed similar performance increases (37-40% on my windows intel i7-6700HQ machine). However this change would hard lock this project to never support HSS (Signal strengths above 15, in vanilla 897 is the limit iirc), just something to consider. By the way, did you somehow verify that redstone behaves the same? While testing with this patch there are about 1% less redstone ticks. Just making sure, but most likely nothing to worry about since there would probably be larger discrepancies if something actually went wrong. Also probably a good idea to add a sanity check and panic if either the default or side input has more than 255 input connections.

Paul1365972 commented 7 months ago

Btw on line 272 of direct.rs there is one constant 16 that should have also been replaced by Self::NUM_QUEUES.

BramOtte commented 7 months ago

Great work! I'm happy to merge this after a quick cargo fmt.

alright I'l do that there is also one other minor fix I will add shortly Also a question when I do cargo fmt another parts of the code also changed I assume I should not include these in my pull request? (this happens in crates/core/src/plot/mod.rs and crates/core/src/world/storage.rs)

StackDoubleFlow commented 7 months ago

It's normally good practice not to, but I'll allow it in this case. Eventually I'd like to have CI fail if code isn't formatted properly so that it stops seeping into the repo.

BramOtte commented 7 months ago

alright that should be it then I also changed ForwardLink::new and tested it with chungus2 and all is fine and well (or at least if you ignore the known issue with chungus2). previously it would crash on non-optimized builds because ss was too large so I made it clamp instead of crash. It might also indicate that something else in the code is going wrong but for now this will suffice.

Paul1365972 commented 7 months ago

Maybe just putting a graph.retain_edges(|g, edge| g[edge].ss <= 15); at the top of the compile function (or in a compiler pass) would make sense here as well.

StackDoubleFlow commented 7 months ago

Maybe just putting a graph.retain_edges(|g, edge| g[edge].ss <= 15); at the top of the compile function (or in a compiler pass) would make sense here as well.

Like in the ClampWeights pass?

StackDoubleFlow commented 7 months ago

Please rebase and I'll merge

Paul1365972 commented 7 months ago

You're right, completly forgot about that pass. In that case I think making the ClampWeights pass mandatory (or just merging it into the InputSearch pass) and keeping the assert would be a good idea, not that it will make much of a difference.

BramOtte commented 7 months ago

Please rebase and I'll merge

I don't really know how rebase works so I merged it instead. From what I understand how rebase works is it is basically equivilant to merging on master. ~~So I created a new branch from the current state of master: https://github.com/BramOtte/MCHPRS/tree/duplicate-signal-strength-rebase aswell and merged my changes onto that incase you find that more convenient.~~ never mind, I pushed another change just merge this branch

StackDoubleFlow commented 7 months ago

Your previous merge commit was empty, there are still conflicts with master.

BramOtte commented 7 months ago

I do not understand what exactly went wrong, but I got the rebase to work, turnsout I just had to force push to make it work. So you should be able to merge with no issues now