-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add in and out degree for states #45
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jeffa5. Some changes will be needed before this can be merged as I'm seeing a significant performance difference. For instance, the 2pc check 10
benchmark increases from ~70 seconds on my current laptop to ~290 seconds.
I didn't profile this to understand where the time is being taken, but I did leave a couple of thoughts in the comments.
Beyond that, my high level assessment is that this might be best implemented as a visitor so that the cost is only paid when someone opts in. e.g. they might opt out for unit tests but then opt in when debugging/etc.
Please let me know what you think and if there's anything I can do to help. If you want to quickly iterate on ideas then we can do that over Discord chat, for instance.
@@ -255,6 +262,11 @@ where M: Model + Send + Sync + 'static, | |||
// Otherwise enqueue newly generated states (with related metadata). | |||
let mut is_terminal = true; | |||
model.actions(&state, &mut actions); | |||
let generated_fingerprint = fingerprint(&state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reuse state_fp
to avoid an extra fingerprint calculation here?
struct NodeData { | ||
in_degree: usize, | ||
out_degree: usize, | ||
parent_fingerprint: Option<Fingerprint>, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing impacting performance is the change from storing Option<Fingerprint>
in the map (which encodes to a single u64
as Fingerprint
is a NonZeroU64
enabling a memory layout optimization) to this new structure that cannot encode as efficiently.
I don't know whether it would help or hurt performance, but perhaps adopting different maps along the lines of https://en.wikipedia.org/wiki/Data-oriented_design is worth exploring.
You might want to use something like https://github.com/TimelyDataflow/differential-dataflow or https://github.com/vmware/database-stream-processor to make the calculations incremental. An example of using the former is in https://github.com/dtolnay/cargo-tally and the latter has some good examples. There is an example for in and out degrees: https://github.com/vmware/database-stream-processor/blob/main/crates/dbsp/examples/degrees.rs |
Adds tracking of the in-degree and out-degree of states in the graph.
This is reported by collecting them all into a
Vec
. This isn't the most efficient but allows full analysis by the (potentially custom) reporter, including calculating percentiles.