Skip to content
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

Issue #8674: Interrupt Work Stealing #8679

Merged
merged 2 commits into from
Aug 25, 2023
Merged

Conversation

hawkfish
Copy link
Contributor

@hawkfish hawkfish commented Aug 24, 2023

Check the work stealing loop for aborts so it doesn't loop forever.

fixes duckdblabs/duckdb-internal#140

Check the work stealing loop for aborts so it doesn't loop forever.
@carlopi
Copy link
Contributor

carlopi commented Aug 24, 2023

Confirm that fixes the issue, and makes sense to me.

I took a look at the code, question is whether this should not use some sort of general locking mechanism to avoid threading issues.

Say 2 threads both call StealWork (that internally has a lock, so will serve only one at a time), then the one that gets out throws right away, would that be also a problem?


edited: I though I had a solution, but it's just adding confusion (and it's wrong). So first maybe we try to check whether this is a problem at all
edit2: NOT A PROBLEM

@hawkfish
Copy link
Contributor Author

hawkfish commented Aug 24, 2023

tasks_remaining is atomic, so it doesn't need a guard, and interrupted is a single byte read, so it doesn't need one either.

I try to avoid locks because they are so heavyweight.

@carlopi
Copy link
Contributor

carlopi commented Aug 24, 2023

tasks_remaining is atomic, so it doesn't need a guard, and interrupted is a single byte read, so it doesn't need one either.

I am not sure that helps. Say two threads both passed the check on !interrupted && tasks_remaining. Then one gets a task and throws, and the other will fail to steal work. What happens then?

@hawkfish
Copy link
Contributor Author

When it fails, it will yield and the next loop will find the interrupt.

@carlopi
Copy link
Contributor

carlopi commented Aug 24, 2023

Right, eventually (even a few cycle laters we don't care), it will eventually get out of the loop. Thanks for walking me through.

@hawkfish
Copy link
Contributor Author

Right, eventually (even a few cycle laters we don't care), it will eventually get out of the loop. Thanks for walking me through.

Yeah its not the most resource-friendly way of doing it, but feature freeze (and VLDB) are approaching, so this seems good enough for now.

Copy link
Contributor

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

lookin good!

Piggyback coverage tweak
@github-actions github-actions bot marked this pull request as draft August 24, 2023 21:43
@hannes hannes marked this pull request as ready for review August 25, 2023 06:45
@hannes hannes merged commit 67f6118 into duckdb:main Aug 25, 2023
37 of 38 checks passed
@hawkfish hawkfish deleted the window-hang branch September 13, 2023 08:14
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.

None yet

4 participants