perf: visit less clauses during propagation #66
Conversation
|
Two flamegraphs, before and after. The timing difference is huge - after this PR it finishes in a second or so, and before it took more than 30 secs before I killed it.
|
|
Here are the benchmark results compared to the main branch:
This is a clear improvement. @jjerphan You might be interested in this one! |
wolfv
changed the title
baszalmstra
changed the title
|
Hi @baszalmstra and @wolfv, At first sight, the performance improvements are quite significant. I am definitely interested in having a look at this and at other ones. I need to find some proper time to review it. On a side note, I would like to implement an ILP formulation of the problem (as first highlighted by @DerThorsten): this offers theoretical consistency but might not offer tractability in practice. I wrote a few notes about it. |
baszalmstra
force-pushed
the
performance/only_add_forbidden_for_visisted_solvables
branch
from
ba5b415
to
838bf88
Compare
| /// Constructs a new [`Literal`] from a [`InternalSolvableId`] and a boolean | ||
| /// indicating whether the literal should be negated. | ||
| pub fn new(solvable_id: InternalSolvableId, negate: bool) -> Self { | ||
| assert!(solvable_id.0 < (u32::MAX >> 1) - 1, "solvable id too big"); |
There was a problem hiding this comment.
Could / should this be a debug assert? I assume we're not constructing so many literals on the fly anymore, so perhaps this is fine?
| @@ -1078,41 +1126,43 @@ impl<D: DependencyProvider, RT: AsyncRuntime> Solver<D, RT> { | |||
| ) -> Result<u32, Conflict> { | |||
| { | |||
| tracing::info!( | |||
| "├─ Propagation conflicted: could not set {solvable} to {attempted_value}", | |||
| "├┬ Propagation conflicted: could not set {solvable} to {attempted_value}", | |||
There was a problem hiding this comment.
This is such beautiful ascii pipes.
This branch adds several optimizations. From large impact to small they are: