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

serverset: Guard inner with a Mutex #8164

Merged

Conversation

@illicitonion
Copy link
Contributor

commented Aug 12, 2019

Before, we used atomics for the Serverset-level synchronisation, and per-Backend locks.

We're about to need Serverset-level locking anyway to add connection limits, so simplify into one Mutex as a prefactoring.

illicitonion added 2 commits Aug 8, 2019
Guard inner in a Mutex
Rather than trying to use atomics. The structure is about to need a lock
anyway.
Avoid per-Backend locks
We can just use the Serverset-global lock now.
let i = self.inner.next.fetch_add(1, Ordering::Relaxed) % server_count;
let server = &self.inner.connections[i];
let i = inner.next % server_count;
inner.next = inner.next.wrapping_add(1);

This comment has been minimized.

Copy link
@blorente

blorente Aug 12, 2019

Contributor

Why is this important to wrapping_add here? Should we wrapping_add everywhere, or is it just because the prior code used modular arithmetic anyway and we want to preserve functionality?

Also, now that I think of it, doesn't the prior code add before taking the index? Not sure it matters a lot, but in the previous code i = (inner.next++) % server_count, where now i = inner.next % server_count; inner.next++.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Aug 12, 2019

Author Contributor

In practice, this gets incremented once per request, so it's unlikely but not impossible that a pants invocation will overflow this. If we don't wrapping_add, we will panic when it overflows (at least, we will in debug mode).

Before we were using AtomicUsize.fetch_add which is specified to do a wrapping addition on overflow: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicUsize.html#method.fetch_add - and fetch_add returns the value before the addition :) These are great additional reasons to avoid using the weird/unfamiliar AtomicUsize!

@illicitonion illicitonion merged commit ac31ea9 into pantsbuild:master Aug 12, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@stuhood stuhood deleted the twitter:dwagnerhall/serverset-connections/mutex branch Aug 22, 2019

@stuhood stuhood added this to the 1.19.x milestone Aug 22, 2019

stuhood added a commit that referenced this pull request Aug 22, 2019
serverset: Guard inner with a Mutex (#8164)
Before, we used atomics for the `Serverset`-level synchronisation, and per-`Backend` locks.

We're about to need `Serverset`-level locking anyway to add connection limits, so simplify into one `Mutex` as a prefactoring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.