Skip to content

solver: speed-up grounding and solve#51174

Merged
haampie merged 2 commits intospack:developfrom
alalazo:solver/speed-up-grounding
Aug 18, 2025
Merged

solver: speed-up grounding and solve#51174
haampie merged 2 commits intospack:developfrom
alalazo:solver/speed-up-grounding

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Aug 18, 2025

This is another PR to speed-up the concretizer, guided by the profiling results from the development version of clingo v6:

  • Use a positive fact concrete(PackageNode), instead of not build(PackageNode)
  • Simplify construction of condition_set given the current set of rules

@alalazo alalazo self-assigned this Aug 18, 2025
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

Benchmarked against:

radiuss.pr.csv
radiuss.develop.csv

radiuss

@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

Why does conduit double in solve time?

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

Why does conduit double in solve time?

No idea, it might be that our heuristic made things worse after this change for that specific spec. Overall, we have a few specs, including conduit or py-deephyper etc., whose solve times are very sensible to changes in the concretizer.

@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

For conduit I see the opposite in solve time. Looks like performance is pretty deterministic for fixed input, just highly dependent on exact input.

before-0.txt:    solve         12.814s
before-1.txt:    solve         12.812s
before-2.txt:    solve         12.817s
--------------------------------------
 after-0.txt:    solve         20.539s
 after-1.txt:    solve         20.581s
 after-2.txt:    solve         20.518s

but

before-0.txt:    ground         5.544s
before-1.txt:    ground         5.577s
before-2.txt:    ground         5.542s
--------------------------------------
 after-0.txt:    ground         5.050s
 after-1.txt:    ground         5.059s
 after-2.txt:    ground         5.044s

I would suggest randomizing the input statements for more useful solve output, cause they typically dominate the overall time, but are pretty meaningless.

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

Looks like performance is pretty deterministic for fixed input, just highly dependent on exact input.

Yes, that's what we discovered when working on #45023 For some unstable specs, slightly changing the order of input facts might change the solve time quite a bit.

For conduit I see the opposite in solve time.

Not sure what you mean here. "before" is ~2x faster than "after" in solve, but grounding improved a bit. If "before" is develop, then it's in line with the outlier in the graph above?

I would suggest randomizing the input statements for more useful solve output, cause they typically dominate the overall time, but are pretty meaningless.

I guess we can do that, though most specs are more stable than conduit, or py-* etc. Grounding times are much more stable, and reducing grounding usually brings perf improvements on average (smaller search space).

@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

Just for reference, on develop with randomized order of input to the solver, I get this for spack solve --timers --fresh conduit. So, a rather long tail, not a normal distribution.

histogram

and same but with hiptt:

histogram

just from these two examples, it doesn't look like conduit is special. The fractions p95/p50 and p5/p50 are closer than I had expected.

@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

Not sure what you mean here

I think I mislabeled the first data :)

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

So, with randomization cherry-picked, 5 jobs per concretization, and a smaller set of specs (cause it takes a long time to get results 🙂 ) :

radiuss.pr.csv
radiuss.develop.csv

radiuss

The black line shows the min/max, bar shows the average

@alalazo alalazo marked this pull request as ready for review August 18, 2025 13:06
@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

bar shows the average

which is skewed by outliers you've plotted ;) median would be better, or a test

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

Median:
radiuss

@haampie
Copy link
Copy Markdown
Member

haampie commented Aug 18, 2025

Running a statistical test wilcoxon(..., alternative="less") on the relative change in median runtime per spec, you do get:

Statistically significant improvements (3 fields):

  • ground_time: p = 0.0039
  • solve_time: p = 0.0078
  • total_time: p = 0.0039

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Aug 18, 2025

Can we merge then?

@haampie haampie merged commit b70a028 into spack:develop Aug 18, 2025
33 checks passed
@alalazo alalazo deleted the solver/speed-up-grounding branch August 18, 2025 17:20
climbfuji pushed a commit to climbfuji/spack that referenced this pull request Aug 22, 2025
Speed up the concretizer, guided by the profiling results from the development version of clingo v6:

- Use a positive fact `concrete(PackageNode)`, instead of `not build(PackageNode)`
- Simplify construction of `condition_set` given the current set of rules 

Signed-off-by: Massimiliano Culpo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants