allow reuse of cached provisional memos within the same cycle iteration during maybe_changed_after

[MichaReiser]

This is the same as #786 but during maybe_changed_after.

validate_same_iteration over pessimistically always returns false if a cycle head is currently being checked with maybe_changed_after because maybe_changed_after never pushes the current query on the stack.

This PR addresses the issue by comparing the cycle head's iteration with the latest memo iteration for that cycle head. This is the same as we already did for a cycle that spans multiple threads.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	3c06ee4
🔍 Latest deploy log	https://app.netlify.com/projects/salsa-rs/deploys/6891b2bc611a5b000864597d

[MichaReiser]

Trying to remember why I explicitly added is_cycle_with_other__thread in 30f6eb4 (#882)

It suggests that it is to fix a panic but there's no failing test :( Maybe it only failed on CI?

[MichaReiser]

Nice, it reproduces on CI (but why only on CI?)

[MichaReiser]

I initially restricted the logic here to only apply when this leads to a cross-thread cycle because the cycle_panic test started to hang on Github actions.

Restricting this logic to multi-threaded cycle does fix the test-hang but it's over restrictive. The alternative fix that I added now (the if below the huge comment) accomplishes the same without making any assumptions about on how many threads the cycle runs.

An alternative fix is to change maybe_changed_after to push the current query in deep_verify_memo. While I think that this would have the nice added benefit that our query stack traces are more accurate (they also show queries running `maybe_changed_after), it does have the significant downside that pushing a new query isn't free and we would have to pay that cost for all queries, not just queries participating in cycles.

That's why I opted for this fix instead.

[codspeed-hq]

CodSpeed Performance Report

Merging #954 will degrade performances by 4.91%

_{Comparing MichaReiser:micha/runaway-maybe-changed-after (3c06ee4) with master (86ca4a9)}

Summary

❌ 1 regressions
✅ 11 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
❌	amortized[Input]	2.3 µs	2.4 µs	-4.91%

[MichaReiser]

The output before the fix:

            "salsa_event(WillExecute { database_key: min_panic(Id(1)) })",
            "salsa_event(WillExecute { database_key: min_panic(Id(2)) })",
            "salsa_event(WillExecute { database_key: min_panic(Id(2)) })", <- unnecesary
            "salsa_event(WillExecute { database_key: min_panic(Id(2)) })", <- unnecesary
            "salsa_event(WillExecute { database_key: min_iterate(Id(0)) })",
            "salsa_event(WillIterateCycle { database_key: min_iterate(Id(0)), iteration_count: IterationCount(1), fell_back: false })",
            "salsa_event(WillExecute { database_key: min_panic(Id(1)) })",
            "salsa_event(WillExecute { database_key: min_panic(Id(2)) })",

[MichaReiser]

No idea what's up with codspeed. This code doesn't exercise any fixpoint iteration logic

[carljm]

Looks good, thank you!

[MichaReiser]

I believe there are a few more bugs in maybe_changed_after related to fixpoint but I'll tackle them in separate PRs:

1. I don't think it's safe to return Unchanged if validate_same_iteration returns true. We simply don't know yet if the value has changed.
2. We should never return Unchanged for a provisional value (here https://github.com/MichaReiser/salsa/blob/c2f4827b512b82842dbc84b1ccc4367500e301ed/src/function/maybe_changed_after.rs#L191-L193)
3. We need to update maybe_changed_after_cold to do the same retry logic as for fetch to correctly handle multi threaded validation, see Multithreaded maybe_changed_after with fixpoint iteration. #952