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

x/sync/semaphore: document ordering of Weighted.Acquire #56910

Open
dsnet opened this issue Nov 22, 2022 · 2 comments
Open

x/sync/semaphore: document ordering of Weighted.Acquire #56910

dsnet opened this issue Nov 22, 2022 · 2 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Nov 22, 2022

I was debugging a sluggish system and the problem came down to the ordering of how Acquire was satisfied.

The current behavior of Weighted.Acquire is that it serves acquisitions in order. This is a reasonable behavior, but it should be documented as it is significant performance implications. In particular, this behavior leads to head-of-line blocking where a large acquisition will prevent smaller acquisitions from making progress. The alternate behavior to satisfy any possible acquisition out-of-order, but that would lead to live-lock situations where a continuous stream of small acquisitions prevent a large acquisition from ever making progress.

My solution ended up splitting the semaphore into two parts:

  • a semaphore for small requests
  • a semaphore for large requests.

However, it was not obvious to me that I should do this since I had originally assumed that the semaphore allowed for out-of-order acquisitions.

@gopherbot gopherbot added this to the Unreleased milestone Nov 22, 2022
@dsnet dsnet removed the Proposal label Nov 23, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 23, 2022
@seankhliao
Copy link
Member

cc @bcmills

@neilotoole
Copy link

I agree that the FIFO behavior of Weighted.Acquire should be documented. I have code that relies upon its FIFO-ness, and changing that behavior would break that code. I understand that Weighted.Acquire doesn't currently make that FIFO guarantee in its docs; I think it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

4 participants