Skip to content

Fix hang in nested fixpoint iteration #871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 21, 2025

This fixes a hang in multithreaded fixpoint iteration where the Memo::provisional_retry retried over and over again because all its cycle heads other than itself were finalized at this point.

This also fixes the panic from the kopf repro listed in #831

Copy link

netlify bot commented May 21, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 9f7fa33
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/682da40e3dbcd20008173617

@MichaReiser MichaReiser requested a review from carljm May 21, 2025 09:40
Copy link

codspeed-hq bot commented May 21, 2025

CodSpeed Performance Report

Merging #871 will degrade performances by 5.34%

Comparing MichaReiser:fix-hang (9f7fa33) with master (2d4321e)

Summary

❌ 3 (👁 3) regressions
✅ 9 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 amortized[Input] 3 µs 3.2 µs -4.59%
👁 amortized[InternedInput] 2.9 µs 3.1 µs -4.72%
👁 amortized[SupertypeInput] 3.6 µs 3.8 µs -5.34%

@MichaReiser MichaReiser force-pushed the fix-hang branch 2 times, most recently from 4140252 to a6d9b6c Compare May 21, 2025 09:53
tracing::debug!(
"Waiting for {head_index:?} results in a cycle, return {database_key_index:?} once all other cycle heads completed to allow the outer cycle to make progress."
);
hit_cycle = true;
Copy link
Contributor Author

@MichaReiser MichaReiser May 21, 2025

Choose a reason for hiding this comment

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

I changed this to early-return in #861 but I think that was incorrect. Not early-returning fixes the ty panic for kopf. I might spend some more time to come up with a test case for it but I might also just leave it at it (I already spent an entire week on this :s)

@MichaReiser MichaReiser marked this pull request as ready for review May 21, 2025 10:01
@MichaReiser
Copy link
Contributor Author

This does not fix all occurences of the cycle iteration panic, I can still reproduce it on pandas-stubs

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.

2 participants