Refactor Busy-Waiting Loops to Improve Efficiency #3123
Conversation
…ation - Replaced busy-waiting loops with appropriate synchronization mechanisms across multiple files. - Improved thread management in `BallThread.java`, `Retry.java`, `RetryExponentialBackoff.java`, and `ServiceExecutor.java`. - Introduced exponential backoff for retry logic in `Retry` and `RetryExponentialBackoff`. - Optimized queue message processing in `ServiceExecutor.java` with better sleep intervals. - Ensured the correctness of functionality through extensive testing.
PR SummaryThis PR refactors busy-waiting loops in several Java files to improve efficiency and resource utilization. It replaces busy-waiting with synchronization mechanisms, optimizes thread management, introduces exponential backoff for retries, and enhances queue message processing. The changes improve resource utilization and responsiveness. Changes
autogenerated by presubmit.ai |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (3)
-
d5864d9: updated
-
8aa0647: Merge branch 'iluwatar:master' into master
-
86efd3c: Refactor busy-waiting loops to improve efficiency and resource utilization
-
Replaced busy-waiting loops with appropriate synchronization mechanisms across multiple files.
-
Improved thread management in
BallThread.java,Retry.java,RetryExponentialBackoff.java, andServiceExecutor.java. -
Introduced exponential backoff for retry logic in
RetryandRetryExponentialBackoff. -
Optimized queue message processing in
ServiceExecutor.javawith better sleep intervals. -
Ensured the correctness of functionality through extensive testing.
Files Processed (14)
- commander/src/main/java/com/iluwatar/commander/Retry.java (1 hunk)
- microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java (4 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/MessageQueue.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/TaskGenerator.java (1 hunk)
- queue-based-load-leveling/src/test/java/com/iluwatar/queue/load/leveling/TaskGenSrvExeTest.java (2 hunks)
- retry/src/main/java/com/iluwatar/retry/Retry.java (1 hunk)
- retry/src/main/java/com/iluwatar/retry/RetryExponentialBackoff.java (1 hunk)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (5 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LoginHandler.java (2 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LogoutHandler.java (1 hunk)
- server-session/src/test/java/com.iluwatar.sessionserver/AppTest.java (1 hunk)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (1 hunk)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (4 hunks)
Actionable Comments (4)
-
retry/src/main/java/com/iluwatar/retry/Retry.java [71-71]
performance: "Improve retry mechanism efficiency."
-
twin/src/main/java/com/iluwatar/twin/BallThread.java [53-62]
possible bug: "Potential race condition in thread synchronization."
-
server-session/src/main/java/com/iluwatar/sessionserver/App.java [98-100]
performance: "Improve session expiration mechanism efficiency."
-
queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java [52-54]
possible bug: "Potential race condition in thread synchronization."
Skipped Comments (1)
-
microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java [80-81]
possible issue: "Add error handling to log collection."
| } while (true); | ||
| } | ||
| } | ||
| executorService.schedule(() -> {}, this.delay, TimeUnit.MILLISECONDS); // Schedule retry without blocking |
There was a problem hiding this comment.
The use of Thread.sleep() for delays in Retry and RetryExponentialBackoff classes is inefficient and can lead to resource wastage. Consider using ScheduledExecutorService for scheduling retries with precise delays without blocking the main thread.
| if (isSuspended) { | ||
| synchronized (lock) { | ||
| lock.wait(); | ||
| } | ||
| } else { | ||
| twin.draw(); | ||
| twin.move(); | ||
| Thread.sleep(250); | ||
| } | ||
| } |
There was a problem hiding this comment.
In BallThread, the busy-waiting loop is replaced with a more efficient approach using lock.wait() and lock.notifyAll(). However, ensure that the lock object is properly synchronized to prevent race conditions.
| LOGGER.info("Session expiration checker started..."); | ||
| Thread.sleep(SESSION_EXPIRATION_TIME); // Sleep for expiration time | ||
| Instant currentTime = Instant.now(); |
There was a problem hiding this comment.
The sessionExpirationTask in App.java uses busy-waiting to check for session expiration. This is inefficient. Consider using a ScheduledExecutorService to schedule the expiration check periodically without busy-waiting.
| synchronized (msgQueue.serviceExecutorWait) { | ||
| msgQueue.serviceExecutorWait.wait(); | ||
| } |
There was a problem hiding this comment.
In ServiceExecutor, the busy-waiting loop is replaced with msgQueue.serviceExecutorWait.wait(). Ensure that the msgQueue.serviceExecutorWait object is properly synchronized to prevent race conditions.
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (1)
- 0d0bb7a: Merge branch 'master' into master
Files Processed (11)
- microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java (4 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/MessageQueue.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java (2 hunks)
- queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/TaskGenerator.java (1 hunk)
- queue-based-load-leveling/src/test/java/com/iluwatar/queue/load/leveling/TaskGenSrvExeTest.java (2 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/App.java (5 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LoginHandler.java (2 hunks)
- server-session/src/main/java/com/iluwatar/sessionserver/LogoutHandler.java (1 hunk)
- server-session/src/test/java/com.iluwatar.sessionserver/AppTest.java (1 hunk)
- twin/src/main/java/com/iluwatar/twin/BallThread.java (1 hunk)
- twin/src/test/java/com/iluwatar/twin/BallThreadTest.java (4 hunks)
Actionable Comments (4)
-
queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/MessageQueue.java [39-39]
possible bug: "Potential race condition in
MessageQueue." -
queue-based-load-leveling/src/main/java/com/iluwatar/queue/load/leveling/ServiceExecutor.java [52-54]
possible bug: "Potential issue with spurious wakeups in
ServiceExecutor." -
server-session/src/main/java/com/iluwatar/sessionserver/App.java [62-62]
possible bug: "Potential race condition in
App." -
twin/src/main/java/com/iluwatar/twin/BallThread.java [45-45]
possible bug: "Potential race condition in
BallThread."
Skipped Comments (2)
-
microservices-log-aggregation/src/main/java/com/iluwatar/logaggregation/LogAggregator.java [131-134]
performance: "Potential performance issue in
bufferWakefunction." -
server-session/src/main/java/com/iluwatar/sessionserver/App.java [91-97]
performance: "Potential performance issue in
sessionExpirationTask."
| @@ -36,6 +36,7 @@ | |||
| public class MessageQueue { | |||
|
|
|||
| private final BlockingQueue<Message> blkQueue; | |||
| public final Object serviceExecutorWait = new Object(); | |||
There was a problem hiding this comment.
In MessageQueue, ensure that serviceExecutorWait is properly synchronized to prevent race conditions when multiple threads access it concurrently.
| synchronized (msgQueue.serviceExecutorWait) { | ||
| msgQueue.serviceExecutorWait.wait(); | ||
| } |
There was a problem hiding this comment.
The wait() call in ServiceExecutor should ideally be within a loop to handle spurious wakeups. This ensures the thread waits until a message is available.
| private static Map<String, Integer> sessions = new HashMap<>(); | ||
| private static Map<String, Instant> sessionCreationTimes = new HashMap<>(); | ||
| private static final long SESSION_EXPIRATION_TIME = 10000; | ||
| private static Object sessionExpirationWait = new Object(); // used to make expiration task wait or work based on event (login request sent or not) |
There was a problem hiding this comment.
In App.java, ensure that sessionExpirationWait is properly synchronized to prevent race conditions when multiple threads access it concurrently.
| @@ -42,37 +42,58 @@ public class BallThread extends Thread { | |||
|
|
|||
| private volatile boolean isRunning = true; | |||
|
|
|||
| private final Object lock = new Object(); | |||
There was a problem hiding this comment.
In BallThread, ensure that the lock object is properly synchronized to prevent race conditions when multiple threads access it concurrently.
What problem does this PR solve?
This PR refactors the busy-waiting loops in the following files:
Server Session / App.javaTwin / BallThread.javaLog Aggregation / LogAggregator.javaCommander / Retry.javaRetry / Retry.javaRetry / RetryExponentialBackoff.javaQueue-Based Load Leveling / ServiceExecutor.java