Skip to content

Simplify full-queue check to use single margin #4426

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: develop
Choose a base branch
from

Conversation

PPLong222
Copy link

Update outdated full-queue check logic of LockFreeTaskQueueCore. No more need to have 2 margin elements; one is sufficient.

Tested in LockFreeTaskQueueStressTest and LockFreeTaskQueueTest

See also #4205

@dkhalanskyjb dkhalanskyjb requested a review from qwwdfsad April 25, 2025 13:20
@qwwdfsad
Copy link
Member

qwwdfsad commented May 6, 2025

I'm a little bit out of capacity right now, and the change is really subtle (aka requires quite a deep dive into git history and queue implementation semantics), so expect some arbitrary delay here. Sorry for the inconvenience!

@PPLong222
Copy link
Author

Thanks a lot for taking the time to reply!

I realise this change is quite subtle and that getting to the bottom of it means digging through git history and the queue’s invariants, so please feel free to review it whenever you have the bandwidth. From what I can tell, the patch shouldn’t affect performance or observable behaviour much—it’s mainly about correctness semantics and avoiding confusion for future readers(or maybe I just have misunderstood something).

The motivation came while I was reading LockFreeTaskQueueCore: I couldn’t understand why the full-queue check uses two extra slots instead of the usual single margin you see in a circular buffer. After spending a day tracing the code and trying a variety of stress-test cases, I still couldn’t find a situation where one margin element isn’t enough.

If I’ve overlooked something, just let me know and I’ll be happy to revise (or close) the PR. Thanks again, and take all the time you need!

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