-
Notifications
You must be signed in to change notification settings - Fork 119
Conversation
Previous implementation of Semaphore64 allows two ways to acquire permits: 1. acquire() - the method will eventually return after it has acquired permits. 2. tryAcquire() - the method returns immediately, reporting whether or not it have acquired the permits. Pubsub implements its own work queue and use tryAcquire exclusively. The problem: To implement acquire(), we must use a lock (I do not know of a better way) and all users must pay for the lock, even if they never use acquire(). In particular, we call notifyAll() when releasing permits; these calls are quite expensive. This commit splits Semaphore64 into two implementations, one exclusively blocks and the other exclusively not. In this way the non-blocking implementation needs not pay for the cost the lock. This reduces CPU usage of pubsub client lib by about 10%.
Codecov Report
@@ Coverage Diff @@
## master #407 +/- ##
============================================
- Coverage 71.7% 71.69% -0.02%
- Complexity 751 753 +2
============================================
Files 155 156 +1
Lines 3354 3363 +9
Branches 261 262 +1
============================================
+ Hits 2405 2411 +6
- Misses 847 848 +1
- Partials 102 104 +2
Continue to review full report at Codecov.
|
return false; | ||
} | ||
|
||
static final class Returning extends Semaphore64 { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I'll hold off on this a bit. Depending on how pubsub simplification goes, this might not add much value. In that case, we might as well not complicate this. |
@garrettjonesgoogle Looks like this will be useful after all. PTAL |
class ReturningSemaphore implements Semaphore64 { | ||
private final AtomicLong currentPermits; | ||
|
||
private static void notNegative(long l) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
import java.util.concurrent.atomic.AtomicLong; | ||
|
||
/** A {@link Sempahore64} that immediately returns with failure if permits are not available. */ | ||
class ReturningSemaphore implements Semaphore64 { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@garrettjonesgoogle PTAL |
LGTM |
Previous implementation of Semaphore64 allows two ways to acquire
permits:
permits.
not it have acquired the permits.
Pubsub implements its own work queue and use tryAcquire exclusively.
The problem:
To implement acquire(), we must use a lock
(I do not know of a better way)
and all users must pay for the lock, even if they never use acquire().
In particular, we call notifyAll() when releasing permits;
these calls are quite expensive.
This commit splits Semaphore64 into two implementations,
one exclusively blocks and the other exclusively not.
In this way the non-blocking implementation needs not pay for the cost
the lock.
This reduces CPU usage of pubsub client lib by about 10%.