You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/09/15 01:05:00 UTC
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
dschneider-pivotal commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r708759656
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
##########
@@ -2292,66 +2270,79 @@ public void destroy() throws CacheWriterException {
* in the HARegionQueue.
*/
@Override
- @edu.umd.cs.findbugs.annotations.SuppressWarnings("TLW_TWO_LOCK_WAIT")
- void checkQueueSizeConstraint() throws InterruptedException {
- if (this.haContainer instanceof HAContainerMap && isPrimary()) { // Fix for bug 39413
- if (Thread.interrupted()) {
- throw new InterruptedException();
- }
- synchronized (this.putGuard) {
- if (putPermits <= 0) {
- synchronized (this.permitMon) {
- if (reconcilePutPermits() <= 0) {
- if (region.getSystem().getConfig().getRemoveUnresponsiveClient()) {
- isClientSlowReceiver = true;
- } else {
- try {
- long logFrequency = CacheClientNotifier.DEFAULT_LOG_FREQUENCY;
- CacheClientNotifier ccn = CacheClientNotifier.getInstance();
- if (ccn != null) { // check needed for junit tests
- logFrequency = ccn.getLogFrequency();
- }
- if ((this.maxQueueSizeHitCount % logFrequency) == 0) {
- logger.warn("Client queue for {} client is full.",
- new Object[] {region.getName()});
- this.maxQueueSizeHitCount = 0;
- }
- ++this.maxQueueSizeHitCount;
- this.region.checkReadiness(); // fix for bug 37581
- // TODO: wait called while holding two locks
- this.permitMon.wait(CacheClientNotifier.eventEnqueueWaitTime);
- this.region.checkReadiness(); // fix for bug 37581
- // Fix for #51400. Allow the queue to grow beyond its
- // capacity/maxQueueSize, if it is taking a long time to
- // drain the queue, either due to a slower client or the
- // deadlock scenario mentioned in the ticket.
- reconcilePutPermits();
- if ((this.maxQueueSizeHitCount % logFrequency) == 1) {
- logger.info("Resuming with processing puts ...");
- }
- } catch (InterruptedException ex) {
- // TODO: The line below is meaningless. Comment it out later
- this.permitMon.notifyAll();
- throw ex;
- }
- }
- }
- } // synchronized (this.permitMon)
- } // if (putPermits <= 0)
- --putPermits;
- } // synchronized (this.putGuard)
+ void waitForPermission() throws InterruptedException {
+ if (!(this.haContainer instanceof HAContainerMap && isPrimary())) {
+ return;
}
+
+ if (Thread.interrupted()) {
+ throw new InterruptedException();
+ }
+
+ long startTime = System.currentTimeMillis();
+ synchronized (this.permitMon) {
+
+ if (putPermits.decrementAndGet() >= 0) {
Review comment:
I think the putPermits.decrementAndGet() call should be before the sync and the startTime. That optimizes the normal case in which the atomic is >= 0. By calling currentTimeMillis first and syncing first we are slowing down threads that are trying to add to a queue that is not full. I think we talked about this before so let me know if you found a problem with this suggestion.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
##########
@@ -2292,66 +2270,79 @@ public void destroy() throws CacheWriterException {
* in the HARegionQueue.
*/
@Override
- @edu.umd.cs.findbugs.annotations.SuppressWarnings("TLW_TWO_LOCK_WAIT")
- void checkQueueSizeConstraint() throws InterruptedException {
- if (this.haContainer instanceof HAContainerMap && isPrimary()) { // Fix for bug 39413
- if (Thread.interrupted()) {
- throw new InterruptedException();
- }
- synchronized (this.putGuard) {
- if (putPermits <= 0) {
- synchronized (this.permitMon) {
- if (reconcilePutPermits() <= 0) {
- if (region.getSystem().getConfig().getRemoveUnresponsiveClient()) {
- isClientSlowReceiver = true;
- } else {
- try {
- long logFrequency = CacheClientNotifier.DEFAULT_LOG_FREQUENCY;
- CacheClientNotifier ccn = CacheClientNotifier.getInstance();
- if (ccn != null) { // check needed for junit tests
- logFrequency = ccn.getLogFrequency();
- }
- if ((this.maxQueueSizeHitCount % logFrequency) == 0) {
- logger.warn("Client queue for {} client is full.",
- new Object[] {region.getName()});
- this.maxQueueSizeHitCount = 0;
- }
- ++this.maxQueueSizeHitCount;
- this.region.checkReadiness(); // fix for bug 37581
- // TODO: wait called while holding two locks
- this.permitMon.wait(CacheClientNotifier.eventEnqueueWaitTime);
- this.region.checkReadiness(); // fix for bug 37581
- // Fix for #51400. Allow the queue to grow beyond its
- // capacity/maxQueueSize, if it is taking a long time to
- // drain the queue, either due to a slower client or the
- // deadlock scenario mentioned in the ticket.
- reconcilePutPermits();
- if ((this.maxQueueSizeHitCount % logFrequency) == 1) {
- logger.info("Resuming with processing puts ...");
- }
- } catch (InterruptedException ex) {
- // TODO: The line below is meaningless. Comment it out later
- this.permitMon.notifyAll();
- throw ex;
- }
- }
- }
- } // synchronized (this.permitMon)
- } // if (putPermits <= 0)
- --putPermits;
- } // synchronized (this.putGuard)
+ void waitForPermission() throws InterruptedException {
+ if (!(this.haContainer instanceof HAContainerMap && isPrimary())) {
+ return;
}
+
+ if (Thread.interrupted()) {
+ throw new InterruptedException();
+ }
+
+ long startTime = System.currentTimeMillis();
+ synchronized (this.permitMon) {
+
+ if (putPermits.decrementAndGet() >= 0) {
+ return;
+ }
+
+ long duration = (System.currentTimeMillis() - startTime);
+
+ if (reconcilePutPermits() >= 0) {
Review comment:
I think the new code is different here because this thread already decremented putPermits and it went negative. In the old code we waited until putPermits was > 0 BEFORE we decremented it. But now we decrement it even if it is already < 0.
reconcilePutPermits updates and returns putPermits so if we see it go positive (>= 0) then this thread now has a permit to add to the queue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org