Skip to content

fix: accumulated values lost when reused tracked fn skips re-execution#1099

Merged
MichaReiser merged 2 commits into
salsa-rs:masterfrom
SAY-5:fix-accumulator-wrongresult-issue-923
Jun 2, 2026
Merged

fix: accumulated values lost when reused tracked fn skips re-execution#1099
MichaReiser merged 2 commits into
salsa-rs:masterfrom
SAY-5:fix-accumulator-wrongresult-issue-923

Conversation

@SAY-5

@SAY-5 SAY-5 commented May 26, 2026

Copy link
Copy Markdown
Contributor

When a tracked fn's inputs haven't changed, maybe_changed_after_cold fast-paths through deep_verify_memo and returns the VerifyResult directly. That result carries the accumulated flag from the fn's edge inputs only; if the fn itself pushed accumulated values, those are ignored. Callers upstream then see Empty and skip traversing this subtree, so accumulated values are silently dropped.

Fix: after deep_verify succeeds, check whether the memo's own AccumulatedMap is non-empty and return Any in that case, matching what the re-execute path already does (lines 229-232).

Fixes #923.

…cute

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@netlify

netlify Bot commented May 26, 2026

Copy link
Copy Markdown

👷 Deploy Preview for salsa-rs processing.

Name Link
🔨 Latest commit 16ab655
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6a1554255469c80008d7267c

@netlify

netlify Bot commented May 26, 2026

Copy link
Copy Markdown

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit b928af4
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/6a1f2ce9f03a4300082a05d7

@codspeed-hq

codspeed-hq Bot commented May 26, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 5.82%

⚡ 1 improved benchmark
✅ 25 untouched benchmarks

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation amortized[InternedInput] 2.1 µs 2 µs +5.82%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing SAY-5:fix-accumulator-wrongresult-issue-923 (b928af4) with master (e4ae47b)

Open in CodSpeed

Comment thread src/function/maybe_changed_after.rs Outdated
Comment on lines +188 to +207
InputAccumulatedValues::Any
} else {
accumulated
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's avoid the unreachable here

Suggested change
}
if let VerifyResult::Unchanged { accumulated } = deep_verify {
// Check if the inputs are still valid. We can just compare `changed_at`.
return Some(if old_memo.revisions.changed_at > revision {
VerifyResult::changed()
} else {
// Propagate accumulated values from inputs *and* from this memo itself.
// Without the own-accumulated check, a caller querying accumulated values
// would see `Empty` and skip traversing into this subtree even though
// this memo directly pushed accumulated values.
VerifyResult::unchanged_with_accumulated(
#[cfg(feature = "accumulator")]
{
if old_memo.revisions.accumulated().is_some() {
InputAccumulatedValues::Any
} else {
accumulated
}

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@SAY-5

SAY-5 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Destructured deep_verify directly so the cfg-gated accumulated binding falls out of the if-let, dropping the unreachable (b928af4). Local cargo test on accumulate*, accumulated_backdate, cycle_accumulate all green with --features accumulator.

@MichaReiser MichaReiser added this pull request to the merge queue Jun 2, 2026
Merged via the queue into salsa-rs:master with commit c3d88eb Jun 2, 2026
12 checks passed
This was referenced Jun 2, 2026
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.

Accumulator misses accumulated values when behind tracked fn

2 participants