Skip to content

Comments

Fix stale tracked struct values in later iterations#1068

Merged
MichaReiser merged 2 commits intosalsa-rs:masterfrom
MichaReiser:create-separate-tracked-structs-in-each-cycle-iteration
Jan 21, 2026
Merged

Fix stale tracked struct values in later iterations#1068
MichaReiser merged 2 commits intosalsa-rs:masterfrom
MichaReiser:create-separate-tracked-structs-in-each-cycle-iteration

Conversation

@MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jan 20, 2026

This PR fixes a cycle handling-related bug where
tracked fields returned stale values from previous iterations if
they share the same hash as a tracked struct created in an earlier iteration.

The root cause of this was that TrackedStruct::update skips updating
if the tracked struct was already written in this revision (which is correct).

However, this causes issues with tracked structs with tracked fields (or hash collisions)
where the tracked struct has different values between two iterations. It's crucial
that the write happens in the later iteration.

The fix here is to not only seed the tracked struct IDs but also seed the disambiguator
map, so that the two tracked structs get different IDs. They're different tracked structs after all
(cycle handling is nothing but unrolling the query ITERATION times).

The downside of this is that using tracked structs in return positions of a cyclic query
is unlikely to ever converge because each iteration creates a new tracked struct with a separate ID.
I don't see a way around this. For those instances, use interned values which erase the identity constraint.

Extracted from #1061

@netlify
Copy link

netlify bot commented Jan 20, 2026

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit e22c36c
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6970d1567f7e050008ff96d3


let result = query_a(&db);

assert_eq!(result, 5);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always returned 1 before

@MichaReiser MichaReiser added the bug Something isn't working label Jan 20, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 20, 2026

CodSpeed Performance Report

Merging this PR will degrade performance by 4.1%

Comparing MichaReiser:create-separate-tracked-structs-in-each-cycle-iteration (e22c36c) with master (2e8e6e9)

Summary

❌ 1 (👁 1) regressed benchmark
✅ 12 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
👁 amortized[SupertypeInput] 2.7 µs 2.8 µs -4.1%

@MichaReiser MichaReiser marked this pull request as ready for review January 20, 2026 09:27
@MichaReiser MichaReiser force-pushed the create-separate-tracked-structs-in-each-cycle-iteration branch from 96c1b3e to 679c500 Compare January 20, 2026 09:59
Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, though it makes tracked structs in fixed-point a lot less useful.

Have you considered disallowing tracked structs in fixed-point entirely?

@MichaReiser
Copy link
Contributor Author

Have you considered disallowing tracked structs in fixed-point entirely?

I haven't, but I think we also reached a state where tracked struct and specify work as expected, even if the caching across revisions might not be ideal, because we delete tracked structs too early.

That's why I'd prefer to keep support for now unless we discover new issues.

@MichaReiser MichaReiser added this pull request to the merge queue Jan 21, 2026
Merged via the queue into salsa-rs:master with commit 5facdfd Jan 21, 2026
12 checks passed
@MichaReiser MichaReiser deleted the create-separate-tracked-structs-in-each-cycle-iteration branch January 21, 2026 13:47
@github-actions github-actions bot mentioned this pull request Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants