Skip to content

Conversation

@konstin
Copy link
Member

@konstin konstin commented Oct 9, 2025

Fixes #373.

By using a Vec instead of an [Fx]HashMap for DependencyConstraints, the insertion order of dependencies becomes deterministic, and thereby our internal prioritization. This fixes the problem in #373 where priorities, and thereby resolutions, can change depending on the platform, such as 64-bit vs 32-bit. Now, the first package in a dependency list is always prioritized.

As DependencyConstraints had exposed the FxHashMap, this is breaking change that requires pubgrub v0.4.

I made SelectedDependencies opaque too for good measure.

See #373 (comment).

This test fails on 32-bit, where the iteration order of an `FxHashMap` is different:

```
$ cargo test -p pubgrub --test tests same_result_across_platforms --target i686-unknown-linux-gnu -q

running 1 test
same_result_across_platforms --- FAILED

failures:

---- same_result_across_platforms stdout ----

thread 'same_result_across_platforms' panicked at tests/tests.rs:137:5:
assertion `left == right` failed
  left: "964"
 right: "712"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    same_result_across_platforms

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 3 filtered out; finished in 0.29s

error: test failed, to rerun pass `-p pubgrub --test tests`
```
If we want to use some else than `FxHashMap` internally at some point.
@konstin konstin changed the title Use stable oder for dependencies Use stable order for dependencies Oct 9, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 9, 2025

CodSpeed Performance Report

Merging #377 will degrade performances by 25.64%

Comparing konsti/dev/use-stable-iteration-order-deps (a1698ae) with dev (c8a9d87)

Summary

❌ 3 regressions
✅ 3 untouched

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Mode Benchmark BASE HEAD Change
Simulation backtracking_singletons 4.3 s 4.5 s -5.03%
Simulation sudoku-easy 3.7 ms 5 ms -25.64%
Simulation sudoku-hard 4.4 ms 5.3 ms -17.95%

@mpizenberg
Copy link
Member

I’d really love to have @Eh2406 point of view on this, especially performance-wise. Because all data structure changes in the lib have potential to significantly degrades performances.

@Eh2406
Copy link
Member

Eh2406 commented Nov 3, 2025

Overall having control over our types is a huge improvement.

For DependencyConstraints Forcing an allocation at all seems a little wasteful. I wish we could take -> impl Iter. The type shenanigans required to pull that off are quite extraordinary, and probably not worth it. But if we have to do an allocation, Vec is not worse that Map. (Although somewhat academically I do wonder why the benchmarks disagree with me.)

@konstin
Copy link
Member Author

konstin commented Nov 4, 2025

I haven't checked, but I assume the sudoku perf regression is due to the changed traversal order, we call get dependencies a max of 999 and realistically much less, I don't see how this could have that much of an impact.

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.

Sort order inconsistent on 32-bit archs

4 participants