Skip to content
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

Make literal picker break activity ties by picking smaller variable #14

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dewert99
Copy link
Contributor

This makes literal picking somewhat more predictable and robust to the order explanations are given by theories (see dewert99/plat-smt#14). Unfortunately it seems this also makes maintaining the order heap more expensive so I'm planning to try to optimize before opening this PR.

@c-cube c-cube marked this pull request as ready for review March 15, 2024 18:48
@c-cube c-cube marked this pull request as draft March 15, 2024 18:48
@dewert99 dewert99 marked this pull request as ready for review March 19, 2024 15:37
@dewert99
Copy link
Contributor Author

I've tried several optimizations:

  • Caching activity levels in the heap
  • Using f32 instead of f64
  • Using a quaternary (4-ary) heap instead of a binary heap
  • Using single u64 comparison instead of a f32 comparison that falls back to a u32 comparison if it is equal
    • This works because all the activity levels are positive so integer comparison is equivalent to floating point comparison.

I'm still not sure if these improvement are sufficient to negate the cost of the change, and some of these improvements could have been done regardless of whether we break ties using variables, although this new ordering scheme does give users more control over the literal picker in case they have extra information about which literals would be better to pick.

@dewert99 dewert99 marked this pull request as draft March 19, 2024 21:38
@dewert99 dewert99 marked this pull request as ready for review March 19, 2024 23:14
@dewert99
Copy link
Contributor Author

I also reversed the order that Vars are processed in cancel_until which brought the performance back to about what it was before, although again that could also have been done using the old ordering scheme.

@dewert99
Copy link
Contributor Author

I also notice that in this and some other my other recent PRs I used the let-else feature, so if/when you publish the next version to crates.io you should probably add rust-version = "1.65.0" to src/batsat/Cargo.toml

@c-cube
Copy link
Owner

c-cube commented Apr 1, 2024

Somewhat stupid question there, but could we sort the conflict before inserting things into the heap? That would make the actual order more resilient, I think?

@dewert99
Copy link
Contributor Author

dewert99 commented Apr 1, 2024

Somewhat stupid question there, but could we sort the conflict before inserting things into the heap? That would make the actual order more resilient, I think?

I think that would help with my original problem, although that may have some cost as well. Breaking ties with variable order also has the advantage of allowing users some control over the initial order. I think the main reason the original picker was so cheap was that it broke ties in a way to do the least amount of heapifying which was helpful when most variables had the same activity level, so these changes would also be helpful when randomizing the initial activity levels/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants