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/10 19:27:34 UTC
[GitHub] [geode] mhansonp opened a new pull request #6858: GEODE-9365: Removing unnecessary synchronization
mhansonp opened a new pull request #6858:
URL: https://github.com/apache/geode/pull/6858
Plus some cleanup
--
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
[GitHub] [geode] agingade commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-921317167
It is about the number of items to keep in the HARegion and then throttle; which can go beyond set limit.
Can we just replace the whole code using capacity and region’s size? and keep it simple...
sync(this) {
if (capacity - region.size()) > 0) {
wait(x time); // Notify when take is done while still waiting...
}
}
--
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
[GitHub] [geode] nabarunnag commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-921192211
also please do write a better commit message during the final commit explaining what has changed : finals added , refactored generics, cleanups of dead comments, new atomic counters.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r707525078
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/ha/HARegionQueue.java
##########
@@ -2211,22 +2194,16 @@ public Object updateHAEventWrapper(InternalDistributedMember sender,
* a single peek thread.
*/
private static class BlockingHARegionQueue extends HARegionQueue {
- /**
- * Guards the Put permits
- */
- private final Object putGuard = new Object();
-
- private final int capacity;
-
/**
* Current put permits available
*/
- private int putPermits;
+ private final AtomicInteger putPermits = new AtomicInteger(0);
Review comment:
Variable changes that were required.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r706478827
##########
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:
Nope that is correct.
--
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
[GitHub] [geode] nabarunnag commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r710414928
##########
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();
+ }
+
+ if (putPermits.decrementAndGet() >= 0) {
Review comment:
Was wondering on the behavior change for value 0, previously if the value was zero, we allowed the code block to execute. Is it ok in the new code not to?
--
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
[GitHub] [geode] mhansonp commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-919355674
> I think you should retrofit the test to use `ExecutorServiceRule` (geode-junit) and `ErrorCollector` (JUnit) instead.
>
> Change all the catch-blocks in threads to hand off the error to the `ErrorCollector` instead of appending to a `StringBuffer`:
>
> ```
> @Rule
> public ErrorCollector errorCollector = new ErrorCollector();
> ```
>
> ```
> } catch (Exception e) {
> errorCollector.addError(e);
> }
> ```
>
> And replace all the `Thread` uses with simple uses of `ExecutorServiceRule`.
>
> Submitting tasks to `ExecutorServiceRule` will return futures the test can wait on (with built in use of GeodeAwaitility timeout) so you can ditch the waiting, any looping recommended by others, etc.
I could use some guidance on this. I will talk to you directly.
--
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
[GitHub] [geode] mhansonp commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-920192523
Updated with a suggested performance change.
--
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
[GitHub] [geode] nabarunnag commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r710417574
##########
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:
just trying to understand the change, previously we alway decremented the putPermits, now we don't allow the block to be executed if the putPermits is zero after decrement but we allowed it earlier.
--
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
[GitHub] [geode] mhansonp merged pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp merged pull request #6858:
URL: https://github.com/apache/geode/pull/6858
--
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
[GitHub] [geode] agingade commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r710524258
##########
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())) {
Review comment:
Based on the observation from recent issues; it will be nice to add comment calling out the constraint check is done with HAContainerMap; bu incase queue configured with eviction (HAContainerRegion) this is not done instead overflown to disk.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r707524590
##########
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) {
+ return;
+ }
+
+ 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.", region.getName());
+ this.maxQueueSizeHitCount = 0;
+ }
+
+ ++this.maxQueueSizeHitCount;
+ this.region.checkReadiness();
+ long millisToWait = CacheClientNotifier.eventEnqueueWaitTime - duration;
+ if (millisToWait < 0) {
+ millisToWait = 0;
+ }
+
+ this.permitMon.wait(millisToWait);
+
+ this.region.checkReadiness();
+
+ reconcilePutPermits();
+ if (((this.maxQueueSizeHitCount % logFrequency) == 1) || (duration > 100)) {
+ logger.info("Resuming with processing puts ... millisToWait = " + millisToWait);
+ }
+
+ } catch (InterruptedException ex) {
+ // TODO: The line below is meaningless. Comment it out later
+ this.permitMon.notifyAll();
+ throw ex;
+ }
+ }
+ } // synchronized (this.permitMon)
+
}
+ /* Do not call this method directly from anywhere except checkQueueSizeConstraint */
+
/**
* This function should always be called under a lock on putGuard & permitMon obejct
*
* @return int current Put permits
*/
private int reconcilePutPermits() {
- putPermits += takeSidePutPermits;
Review comment:
additional pieces required.
--
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
[GitHub] [geode] jchen21 commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r708746635
##########
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 this should be `if (reconcilePutPermits() > 0) {` without the `=`. Because the original code that you deleted is `if (reconcilePutPermits() <= 0) {`.
--
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
[GitHub] [geode] mhansonp commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-921283424
> also please do write a better commit message during the final commit explaining what has changed : finals added , refactored generics, cleanups of dead comments, new atomic counters.
I kind of pointed that stuff out in the comments I added when I created this PR, but point taken. I will do a better job of that in the future.
--
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
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [geode] jchen21 commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r706441484
##########
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:
getAndDecrement()?
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/cache/ha/BlockingHARegionJUnitTest.java
##########
@@ -281,26 +231,15 @@ public void testConcurrentPutsTakesNotExceedingLimit() throws Exception {
join(thread9, 30 * 1000);
join(thread10, 30 * 1000);
- WaitCriterion ev = new WaitCriterion() {
- @Override
- public boolean done() {
- return hrq.region.size() == 20000;
- }
+ GeodeAwaitility.await().until(() -> hrq.region.size() == 20000);
- @Override
- public String description() {
- return null;
- }
- };
- GeodeAwaitility.await().untilAsserted(ev);
+ assertThat(thread1.isAlive()).isTrue();
Review comment:
Why not using a loop here?
--
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
[GitHub] [geode] nabarunnag commented on pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on pull request #6858:
URL: https://github.com/apache/geode/pull/6858#issuecomment-921189732
Is there a behavior change in this code ? just wondering if there is a need for a test to be written?
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r707524403
##########
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")
Review comment:
This is the main body of the change.
--
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
[GitHub] [geode] mhansonp commented on a change in pull request #6858: GEODE-9365: Removing unnecessary synchronization
Posted by GitBox <gi...@apache.org>.
mhansonp commented on a change in pull request #6858:
URL: https://github.com/apache/geode/pull/6858#discussion_r706479441
##########
File path: geode-core/src/integrationTest/java/org/apache/geode/internal/cache/ha/BlockingHARegionJUnitTest.java
##########
@@ -281,26 +231,15 @@ public void testConcurrentPutsTakesNotExceedingLimit() throws Exception {
join(thread9, 30 * 1000);
join(thread10, 30 * 1000);
- WaitCriterion ev = new WaitCriterion() {
- @Override
- public boolean done() {
- return hrq.region.size() == 20000;
- }
+ GeodeAwaitility.await().until(() -> hrq.region.size() == 20000);
- @Override
- public String description() {
- return null;
- }
- };
- GeodeAwaitility.await().untilAsserted(ev);
+ assertThat(thread1.isAlive()).isTrue();
Review comment:
The test is disabled. I chose not to modify it.
--
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