Duplicate signal strength to all outputs #129

BramOtte · 2023-10-15T11:43:16Z

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 · 2023-11-23T20:38:19Z

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 · 2023-11-24T17:13:46Z

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

StackDoubleFlow

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

crates/core/src/redpiler/backend/direct.rs

StackDoubleFlow · 2023-12-05T23:09:21Z

crates/core/src/redpiler/backend/direct.rs

@@ -565,33 +599,26 @@ fn schedule_tick(
    scheduler.schedule_tick(node_id, delay, priority);
 }

-fn link_strength(link: DirectLink, nodes: &Nodes) -> u8 {
-    nodes[link.to].output_power.saturating_sub(link.weight)
+const INPUT_MASK: u128 = u128::from_ne_bytes([0, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255]);


nit: maybe a better name being BOOL_INPUT_MASK?
Better to be specific since it's module scoped.

BramOtte · 2023-12-05T23:28:45Z

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 · 2023-12-05T23:32:43Z

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 · 2023-12-05T23:36:55Z

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 · 2023-12-06T01:22:31Z

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 · 2023-12-08T01:53:26Z

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 · 2023-12-08T01:53:52Z

Please rebase and I'll merge

Paul1365972 · 2023-12-08T02:28:05Z

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 · 2023-12-08T19:57:35Z

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 · 2023-12-11T16:20:20Z

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

BramOtte · 2023-12-11T18:07:05Z

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

crates/core/src/redpiler/backend/direct.rs

This allows input states to be checked with simd because the input nodes don't need to be fetched

when creating a forward link

This isn't really neccesary right now but if we ever add optimizations which combine nodes it might become likely that a node will have more than 255 inputs.

* decrease size of DirectLink to 4 bytes * duplicate signal strength to all outputs This allows input states to be checked with simd because the input nodes don't need to be fetched * cleanup old code * remove erroring line * make index of NodeId private again * Make TickScheduler::NUM_QUEUES constant * remove from_ne_bytes from last_index_positive * only get needed repeater inputs * replace magic number with NUM_QUEUES * don't crash the program when ss is larger than 15 when creating a forward link * fix minor typo * rename INPUT_MASK to BOOL_INPUT_MASK * Run cargo fmt * address nits * Add check for maximum inputs. This isn't really neccesary right now but if we ever add optimizations which combine nodes it might become likely that a node will have more than 255 inputs. * make poper use of input counters in from_compile_node * Make clamp_weights pass mandatory * Use get_unchecked_mut() for incrementing signal strength counters * skip updating if output power did not change * wrap input arrays into a struct to ensure they are aligned * run cargo fmt

StackDoubleFlow reviewed Dec 5, 2023

View reviewed changes

BramOtte force-pushed the some-optimizations branch from f995fba to 5f9d510 Compare December 11, 2023 17:57

StackDoubleFlow requested changes Dec 14, 2023

View reviewed changes

crates/core/src/redpiler/backend/direct.rs Outdated Show resolved Hide resolved

BramOtte added 15 commits December 14, 2023 17:56

decrease size of DirectLink to 4 bytes

263672c

duplicate signal strength to all outputs

da448b4

This allows input states to be checked with simd because the input nodes don't need to be fetched

cleanup old code

41f830c

remove erroring line

f1f3806

make index of NodeId private again

c4cce07

Make TickScheduler::NUM_QUEUES constant

5ec5e71

remove from_ne_bytes from last_index_positive

c7edd2f

only get needed repeater inputs

4c3e3ad

replace magic number with NUM_QUEUES

9994ba0

don't crash the program when ss is larger than 15

3026677

when creating a forward link

fix minor typo

20df862

rename INPUT_MASK to BOOL_INPUT_MASK

70e5b43

Run cargo fmt

5b3a035

address nits

0d8b931

Add check for maximum inputs.

ffe264c

This isn't really neccesary right now but if we ever add optimizations which combine nodes it might become likely that a node will have more than 255 inputs.

BramOtte added 6 commits December 14, 2023 17:56

make poper use of input counters in from_compile_node

2120aa5

Make clamp_weights pass mandatory

ba7042d

Use get_unchecked_mut() for incrementing signal strength counters

4bca713

skip updating if output power did not change

ee302e9

wrap input arrays into a struct to ensure they are aligned

a7e79cc

run cargo fmt

9c9e22f

BramOtte force-pushed the some-optimizations branch from 56a75a0 to 9c9e22f Compare December 14, 2023 17:01

StackDoubleFlow approved these changes Dec 14, 2023

View reviewed changes

StackDoubleFlow merged commit 6ae0623 into MCHPR:master Dec 14, 2023
1 check passed

StackDoubleFlow mentioned this pull request Dec 14, 2023

Add noteblocks and instruments #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Duplicate signal strength to all outputs #129

Duplicate signal strength to all outputs #129

BramOtte commented Oct 15, 2023 •

edited

Loading

Paul1365972 commented Nov 23, 2023

Paul1365972 commented Nov 24, 2023

StackDoubleFlow left a comment

StackDoubleFlow Dec 5, 2023

BramOtte commented Dec 5, 2023 •

edited

Loading

StackDoubleFlow commented Dec 5, 2023

BramOtte commented Dec 5, 2023 •

edited

Loading

Paul1365972 commented Dec 6, 2023

StackDoubleFlow commented Dec 8, 2023

StackDoubleFlow commented Dec 8, 2023

Paul1365972 commented Dec 8, 2023

BramOtte commented Dec 8, 2023 •

edited

Loading

StackDoubleFlow commented Dec 11, 2023

BramOtte commented Dec 11, 2023

Duplicate signal strength to all outputs #129

Duplicate signal strength to all outputs #129

Conversation

BramOtte commented Oct 15, 2023 • edited Loading

Paul1365972 commented Nov 23, 2023

Paul1365972 commented Nov 24, 2023

StackDoubleFlow left a comment

Choose a reason for hiding this comment

StackDoubleFlow Dec 5, 2023

Choose a reason for hiding this comment

BramOtte commented Dec 5, 2023 • edited Loading

StackDoubleFlow commented Dec 5, 2023

BramOtte commented Dec 5, 2023 • edited Loading

Paul1365972 commented Dec 6, 2023

StackDoubleFlow commented Dec 8, 2023

StackDoubleFlow commented Dec 8, 2023

Paul1365972 commented Dec 8, 2023

BramOtte commented Dec 8, 2023 • edited Loading

StackDoubleFlow commented Dec 11, 2023

BramOtte commented Dec 11, 2023

BramOtte commented Oct 15, 2023 •

edited

Loading

BramOtte commented Dec 5, 2023 •

edited

Loading

BramOtte commented Dec 5, 2023 •

edited

Loading

BramOtte commented Dec 8, 2023 •

edited

Loading