Skip to content

Simplify WaitGroup implementation #958

Merged
MichaReiser merged 2 commits into
salsa-rs:masterfrom
Veykril:veykril/push-rqyrkyttxpol
Oct 20, 2025
Merged

Simplify WaitGroup implementation #958
MichaReiser merged 2 commits into
salsa-rs:masterfrom
Veykril:veykril/push-rqyrkyttxpol

Conversation

@Veykril

@Veykril Veykril commented Aug 3, 2025

Copy link
Copy Markdown
Member

No description provided.

@netlify

netlify Bot commented Aug 3, 2025

Copy link
Copy Markdown

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 9b4da36
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/68f610e0389171000802e4d7

@codspeed-hq

codspeed-hq Bot commented Aug 3, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #958 will degrade performances by 5.28%

Comparing Veykril:veykril/push-rqyrkyttxpol (9b4da36) with master (9cfe41c)

Summary

❌ 1 (👁 1) regression
✅ 12 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 amortized[Input] 2.1 µs 2.2 µs -5.28%

@MichaReiser

Copy link
Copy Markdown
Contributor

Is there any other benefit than code size reduction. It seems codspeed doesn't like it at all.

@Veykril Veykril force-pushed the veykril/push-rqyrkyttxpol branch from 7a3884e to ef1c5dc Compare August 3, 2025 08:02
@Veykril

Veykril commented Aug 3, 2025

Copy link
Copy Markdown
Member Author

No there isn't. I was curious about the perf impact, I agree this isn't great. Checking if we can actually simplify our impl now

@Veykril Veykril force-pushed the veykril/push-rqyrkyttxpol branch from ef1c5dc to 1b15e41 Compare August 3, 2025 08:02
@Veykril Veykril changed the title Replace manual WaitGroup impl with crossbeam's Simplify WaitGroup implementation Aug 3, 2025
@Veykril

Veykril commented Aug 3, 2025

Copy link
Copy Markdown
Member Author

Hm, we could get rid of the additional Arc as well and re-use the Zalsa arc but that makes the code a lot more ugly (thanks to having to deal with drop order), so this is the best I can come up with. Trading an unwrap with unsafe solely due to polonius borrow checking not being a thing yet :(

@MichaReiser MichaReiser requested a review from ibraheemdev August 5, 2025 07:49
@MichaReiser

Copy link
Copy Markdown
Contributor

@ibraheemdev could you take a look at this PR?

Comment thread src/storage.rs Outdated
Co-authored-by: Ibraheem Ahmed <ibraheem@ibraheem.ca>
@Veykril Veykril force-pushed the veykril/push-rqyrkyttxpol branch from ee184dd to 9b4da36 Compare October 20, 2025 10:37
@Veykril Veykril added this pull request to the merge queue Oct 20, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 20, 2025
@MichaReiser MichaReiser added this pull request to the merge queue Oct 20, 2025
Merged via the queue into salsa-rs:master with commit a4113cd Oct 20, 2025
10 of 12 checks passed
@github-actions github-actions Bot mentioned this pull request Oct 20, 2025
@ChayimFriedman2

Copy link
Copy Markdown
Contributor

This introduced a potential to deadlock. I'm still investigating how exactly, but my guess is that the atomic ordering is not correct (it's not at least in one thing, but that doesn't fix the deadlock).

ChayimFriedman2 added a commit to ChayimFriedman2/salsa that referenced this pull request Dec 16, 2025
This PR can create deadlocks - there is nothing synchronizing the mutex/condvar with the `Arc` of the `Zalsa`, therefore code may wake up by the `Condvar` but not see an updated refcount, go to sleep again, and never wake up.
github-merge-queue Bot pushed a commit that referenced this pull request Dec 17, 2025
This PR can create deadlocks - there is nothing synchronizing the mutex/condvar with the `Arc` of the `Zalsa`, therefore code may wake up by the `Condvar` but not see an updated refcount, go to sleep again, and never wake up.
@github-actions github-actions Bot mentioned this pull request Dec 17, 2025
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.

4 participants