refactor: stop allocating on tuple and list hashing by robinbb · Pull Request #14542 · ocaml/dune

robinbb · 2026-05-15T02:31:31Z

Summary

Extract the (acc * 31) + x hash accumulator into a new Stdune.Hash module per #14542 review, and route Tuple.T2.hash, Tuple.T3.hash, List.hash, Result.hash, and Resolve.error_hash through it. None of these allocate per call any more. (Option.hash is already optimal on main as of #14543 and is left untouched.)

Drive-by fix: Stdune.Result.hash no longer collides Ok x with Error y whenever h1 x = h2 y (e.g. Ok 0 vs Error 0) — same bug shape Stdune.Option.hash had before #14543.

Also consumes #14605 directly at one site: src/source/opam_switch.ml already included Repr.Poly, but a redundant manual let hash shadowed the inherited one. Now dropped.

A broader sweep of "eliminate manual hash via Repr.Poly" is out of scope here. Repr.Poly.validate_compare rejects Repr.view, which is pervasive in dune's wrapped-string newtypes (paths, lib-names, opam URLs, checksums, package names, ...). The set of types whose Repr.t is record/variant + primitives all the way down is small enough that the broader cleanup is best done as a separate, deliberate PR with its own decisions about whether to drop Repr.view from the affected newtypes.

Related: #14605 (Repr.Poly.hash).

Copilot

Pull request overview

This PR refactors Stdune tuple and list hash helpers to avoid building intermediate tuples/lists before hashing, replacing polymorphic hashing with arithmetic combiners.

Changes:

Replaced Tuple.T2.hash and Tuple.T3.hash tuple allocation with multiplicative integer combination.
Replaced List.hash’s mapped-list allocation with a left fold.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`otherlibs/stdune/src/tuple.ml`	Updates tuple hash implementations to combine component hashes directly.
`otherlibs/stdune/src/list.ml`	Updates list hashing to fold over element hashes without constructing an intermediate list.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

robinbb · 2026-05-15T03:28:12Z

Allocation measurement

To check the magnitude of the effect on dune itself, ran a paired dune-on-dune null build experiment.

Setup. dune-old built from upstream/main at 080d3e680d (parent of this PR's commits); dune-new built from this PR's tip c566d4445a. Workload: a clone of the dune source tree, fully built — dune build is a no-op rebuild that exercises the memo cache scan and rule-evaluation path without doing compilation work. Measurement: OCAMLRUNPARAM=v=0x400 <dune> build, picking the parent-process allocation block by max allocated_words. 5 paired runs alternating old/new.

Results.

run	old `allocated_words`	new `allocated_words`	Δ
1	757,503,985	756,686,557	−817,428
2	757,587,297	756,756,752	−830,545
3	757,503,216	756,687,417	−815,799
4	757,655,668	756,604,684	−1,050,984
5	757,587,355	756,759,976	−827,379

Mean Δ: −868,427 words ≈ −7.0 MB per null build (−0.115%). All 5 paired deltas have the same sign; range of deltas (~235k) is well below the magnitude (~870k). minor_collections and major_collections were unchanged between binaries, so the effect is purely on minor-heap throughput, not GC pressure.

Treating this as a defensive/clarity change rather than a perf win — the saving is real but unlikely to be observable in wall time on a realistic workload.

## Summary `Option.hash f None` and `Option.hash f (Some x)` previously both went through `Stdlib.Hashtbl.hash`, which gave equal results whenever `f x = 0` because `None` shares OCaml's tag-0 runtime representation with the integer 0. So under stdune's existing hashers: - `Option.hash Bool.hash (Some false)` collided with `Option.hash Bool.hash None` (`Bool.hash false = 0`). - `Option.hash Unit.hash (Some ())` collided with `Option.hash Unit.hash None` (`Unit.hash () = 0`). This PR replaces the structural hash with a tagged recurrence — `None -> 0`, `Some s -> (f s * 31) + 1` — so the two cases land in disjoint buckets. Added an expect test in `otherlibs/stdune/test/option_tests.ml` covering the `Bool` and `Unit` cases that previously collided. Discovered while surveying allocation-heavy hash sites for a follow-up to #14542. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

rgrinberg · 2026-05-15T17:28:28Z

It's ok to deviate from Poly.hash. We do not rely on hashes for correctness, only for performance. However, I'd rather not introduce code like this unless we can measure the impact on dune itself.

robinbb · 2026-05-15T17:37:40Z

It's ok to deviate from Poly.hash. We do not rely on hashes for correctness, only for performance. However, I'd rather not introduce code like this unless we can measure the impact on dune itself.

I think I have done that, at least for dune-on-dune builds. It has an almost-imperceptible performance increase of 0.1%, and a reduction in memory allocations of 0.1%, also. An admittedly minor improvement. Simply close the PR if you don't want the change, @rgrinberg. I'm not concerned either way. Just offering a small improvement in performance; I understand it comes with risks related to hash bits entropy.

rgrinberg · 2026-05-19T16:55:47Z

Here's a related for PR that allows us to replace poorly written hash functions with a Poly.hash #14605

For this PR, I'm ok with the approach in principle, but I would prefer the underlying implementation to live in a separate module. E.g. Stdune.Hash with:

type t
val create : unit -> t
val feed : t -> int -> t
val hash : t -> int

Extract the multiplicative `(acc * 31) + x` accumulator into a new `Stdune.Hash` module with the API requested in review: type t val create : unit -> t val feed : t -> int -> t val hash : t -> int Route the structural hash combinators through it: - `Stdune.Tuple.T2.hash`, `Stdune.Tuple.T3.hash`, `Stdune.List.hash` drop the allocations (a fresh inner tuple / a fresh `int list`) that they previously paid before handing off to `Hashtbl.hash`. - `Stdune.Result.hash` likewise stops allocating and gains explicit discriminator bytes. This is a drive-by bugfix: the previous implementation collided `Ok x` with `Error y` whenever the inner hashes were equal (e.g. `Ok 0` and `Error 0`), the same shape as the bug `Stdune.Option.hash` had pre-ocaml#14543. - `Resolve.error_hash` (`src/dune_rules/resolve.ml`) drops its 2-tuple allocation and its per-call `List.map ~f:Lazy.force` (which builds a fresh `int list` of length |stack_frames|), folding the forced frames into the accumulator instead. `Stdune.Option.hash` is already optimal on main as of ocaml#14543 and is left unchanged. Direct consumption of ocaml#14605: `src/source/opam_switch.ml` already included `Repr.Poly`, but a redundant manual `let hash` shadowed the inherited one. That definition is now dropped. This is a single demonstration site for the elimination-by-Repr.Poly direction referenced in the review; a broader sweep is out of scope here because `Repr.Poly`'s `validate_compare` rejects `Repr.view`, which pervades dune's wrapped-string newtypes (paths, lib-names, opam URLs, checksums, package names, ...). Same `'a -> 'a -> int` callsite API; callers don't change. Hash values shift, but these functions feed `Hashtbl` / `Memo` bucket selectors only — no persistent serialization, no cross-process invariants, so the shift is invisible to consumers. Signed-off-by: Robin Bate Boerop <me@robinbb.com>

robinbb · 2026-05-29T22:55:28Z

@rgrinberg Is what's in this PR now what you had in mind? (Forgive my slowness getting back to this, but I think it's low priority for both of us.)

robinbb self-assigned this May 15, 2026

robinbb requested a review from Copilot May 15, 2026 02:38

Copilot started reviewing on behalf of robinbb May 15, 2026 02:39 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread otherlibs/stdune/src/list.ml Outdated

Comment thread otherlibs/stdune/src/list.ml Outdated

robinbb force-pushed the robinbb-stdune-hash-nonalloc branch from cc6bccb to e193cc5 Compare May 15, 2026 02:54

robinbb requested a review from Copilot May 15, 2026 02:55

Copilot started reviewing on behalf of robinbb May 15, 2026 02:56 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread otherlibs/stdune/src/list.ml Outdated

robinbb mentioned this pull request May 15, 2026

fix: Option.hash distinguishes None from Some x when (f x = 0) #14543

Merged

robinbb force-pushed the robinbb-stdune-hash-nonalloc branch from e193cc5 to c566d44 Compare May 15, 2026 03:08

robinbb requested a review from Copilot May 15, 2026 03:09

Copilot started reviewing on behalf of robinbb May 15, 2026 03:10 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread otherlibs/stdune/src/list.ml Outdated

robinbb requested a review from rgrinberg May 15, 2026 16:15

robinbb assigned rgrinberg May 15, 2026

robinbb marked this pull request as ready for review May 15, 2026 16:16

robinbb force-pushed the robinbb-stdune-hash-nonalloc branch 3 times, most recently from 71fff16 to 7f6dc75 Compare May 29, 2026 22:49

robinbb force-pushed the robinbb-stdune-hash-nonalloc branch from 7f6dc75 to 0bf3b93 Compare May 29, 2026 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: stop allocating on tuple and list hashing#14542

refactor: stop allocating on tuple and list hashing#14542
robinbb wants to merge 1 commit into
ocaml:mainfrom
robinbb:robinbb-stdune-hash-nonalloc

robinbb commented May 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

robinbb commented May 15, 2026

Uh oh!

rgrinberg commented May 15, 2026

Uh oh!

robinbb commented May 15, 2026

Uh oh!

rgrinberg commented May 19, 2026

Uh oh!

robinbb commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

robinbb commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

robinbb commented May 15, 2026

Allocation measurement

Uh oh!

rgrinberg commented May 15, 2026

Uh oh!

robinbb commented May 15, 2026

Uh oh!

rgrinberg commented May 19, 2026

Uh oh!

robinbb commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robinbb commented May 15, 2026 •

edited

Loading