Implement SIMD Optimization for Vector Classes #106#131
Implement SIMD Optimization for Vector Classes #106#131
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/org/fungover/breeze/control/RetryExecutorTest.java (2)
33-33: Translate Swedish comment to English for consistency.The comment "Vi förväntar oss att ett undantag kastas eftersom alla retries misslyckas" should be in English.
- // Vi förväntar oss att ett undantag kastas eftersom alla retries misslyckas + // We expect an exception to be thrown since all retries fail
73-79: Improve test state management.Using a mutable array to track attempts is not a clean testing practice. Consider using an AtomicInteger for better thread safety and cleaner code.
- final int[] attempts = {0}; + final AtomicInteger attempts = new AtomicInteger(0); String result = RetryExecutor.executeWithRetry(() -> { - if (attempts[0]++ < 2) throw new ServerBusyException(); + if (attempts.getAndIncrement() < 2) throw new ServerBusyException(); - return "Success after " + attempts[0] + " tries"; + return "Success after " + attempts.get() + " tries"; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/fungover/breeze/client/ServerClient.java(1 hunks)src/main/java/org/fungover/breeze/control/RetryExecutor.java(4 hunks)src/main/java/org/fungover/breeze/control/ServerBusyException.java(1 hunks)src/test/java/org/fungover/breeze/control/RetryExecutorTest.java(2 hunks)
🔇 Additional comments (3)
src/main/java/org/fungover/breeze/control/ServerBusyException.java (1)
4-12: LGTM! Well-structured exception class.The class follows Java exception class conventions and provides clear, descriptive messages.
src/main/java/org/fungover/breeze/control/RetryExecutor.java (2)
83-83: LGTM! Good implementation of exponential backoff with jitter.The jitter implementation helps prevent the "thundering herd" problem in distributed systems.
1-1: PR title does not match the implemented changes.The PR title mentions "SIMD Optimization for Vector Classes" but the changes implement retry logic with exponential backoff. Please update the PR title to accurately reflect the changes.
Likely an incorrect or invalid review comment.
| public String fetchData() throws Exception { | ||
| return RetryExecutor.executeWithRetry(() -> { | ||
| // Add actual server call logic here | ||
| if (Math.random() < 0.7) { | ||
| throw new ServerBusyException(); | ||
| } | ||
| return "Sample Data"; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve testability and configurability of the server busy simulation.
The current implementation has several areas for improvement:
- Using
Math.random()makes testing unpredictable. - The probability threshold (0.7) is hard-coded.
Consider this refactoring:
public class ServerClient {
+ private final double busyProbability;
+ private final Random random;
+
+ public ServerClient(double busyProbability) {
+ this.busyProbability = busyProbability;
+ this.random = new Random();
+ }
+
public String fetchData() throws Exception {
return RetryExecutor.executeWithRetry(() -> {
// Add actual server call logic here
- if (Math.random() < 0.7) {
+ if (random.nextDouble() < busyProbability) {
throw new ServerBusyException();
}
return "Sample Data";
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String fetchData() throws Exception { | |
| return RetryExecutor.executeWithRetry(() -> { | |
| // Add actual server call logic here | |
| if (Math.random() < 0.7) { | |
| throw new ServerBusyException(); | |
| } | |
| return "Sample Data"; | |
| }); | |
| } | |
| public class ServerClient { | |
| private final double busyProbability; | |
| private final Random random; | |
| public ServerClient(double busyProbability) { | |
| this.busyProbability = busyProbability; | |
| this.random = new Random(); | |
| } | |
| public String fetchData() throws Exception { | |
| return RetryExecutor.executeWithRetry(() -> { | |
| // Add actual server call logic here | |
| if (random.nextDouble() < busyProbability) { | |
| throw new ServerBusyException(); | |
| } | |
| return "Sample Data"; | |
| }); | |
| } | |
| } |
| public static <T> T executeWithRetry(RiskyOperation<T> operation) throws Exception { | ||
| return builder() | ||
| .maxAttempts(5) | ||
| .exponentialBackoff(1000, 16000) // 1s, 2s, 4s, 8s, 16s | ||
| .retryOn(ServerBusyException.class) | ||
| .onExhaustion(ex -> { | ||
| System.out.println("Final attempt failed after exponential backoff"); | ||
| return true; | ||
| }) | ||
| .build() | ||
| .execute(operation); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve configurability and logging in executeWithRetry.
The method has hard-coded values and uses System.out for logging.
+ private static final Logger logger = LoggerFactory.getLogger(RetryExecutor.class);
+
public static <T> T executeWithRetry(RiskyOperation<T> operation) throws Exception {
return builder()
- .maxAttempts(5)
- .exponentialBackoff(1000, 16000) // 1s, 2s, 4s, 8s, 16s
+ .maxAttempts(RetryConfig.DEFAULT_MAX_ATTEMPTS)
+ .exponentialBackoff(RetryConfig.DEFAULT_INITIAL_DELAY, RetryConfig.DEFAULT_MAX_DELAY)
.retryOn(ServerBusyException.class)
.onExhaustion(ex -> {
- System.out.println("Final attempt failed after exponential backoff");
+ logger.warn("Final attempt failed after exponential backoff", ex);
return true;
})
.build()
.execute(operation);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static <T> T executeWithRetry(RiskyOperation<T> operation) throws Exception { | |
| return builder() | |
| .maxAttempts(5) | |
| .exponentialBackoff(1000, 16000) // 1s, 2s, 4s, 8s, 16s | |
| .retryOn(ServerBusyException.class) | |
| .onExhaustion(ex -> { | |
| System.out.println("Final attempt failed after exponential backoff"); | |
| return true; | |
| }) | |
| .build() | |
| .execute(operation); | |
| } | |
| private static final Logger logger = LoggerFactory.getLogger(RetryExecutor.class); | |
| public static <T> T executeWithRetry(RiskyOperation<T> operation) throws Exception { | |
| return builder() | |
| .maxAttempts(RetryConfig.DEFAULT_MAX_ATTEMPTS) | |
| .exponentialBackoff(RetryConfig.DEFAULT_INITIAL_DELAY, RetryConfig.DEFAULT_MAX_DELAY) | |
| .retryOn(ServerBusyException.class) | |
| .onExhaustion(ex -> { | |
| logger.warn("Final attempt failed after exponential backoff", ex); | |
| return true; | |
| }) | |
| .build() | |
| .execute(operation); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/fungover/breeze/control/RetryExecutor.java (3)
33-35: Avoid wrapping non-retryable exceptions withIllegalArgumentException.Throwing an
IllegalArgumentExceptionfor non-retryable exceptions can obscure their real cause. Consider either rethrowing the original exception or introducing a more descriptive custom exception explaining it’s not retryable.- throw new IllegalArgumentException("Operation failed", ex); + throw ex; // or wrap with a custom NonRetryableOperationException
47-50: Ensure the final return statement is reachable or remove as dead code.Because the loop always throws if retries are exhausted, the
return null;statement at line 50 is effectively unreachable. Remove this statement or justify under which condition it should returnnull.
105-116: Allow external configuration for default retry parameters.Hard-coded defaults (5 attempts, 1s to 16s delay range,
ServerBusyExceptionas the retry type) can be limiting for broader usage. Offer a method or builder config to override these so users can provide different defaults without duplicating code.src/main/java/org/fungover/breeze/client/ServerClient.java (1)
25-30: Use realistic server call logic or make it configurable for testing.The placeholder
"Sample Data"return is fine for demonstrations. However, consider injecting a service or callback for actual data retrieval. This makes testing and production usage more flexible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/fungover/breeze/client/ServerClient.java(1 hunks)src/main/java/org/fungover/breeze/control/RetryConfig.java(1 hunks)src/main/java/org/fungover/breeze/control/RetryExecutor.java(1 hunks)src/test/java/org/fungover/breeze/control/RetryExecutorTest.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/fungover/breeze/control/RetryConfig.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/fungover/breeze/control/RetryExecutorTest.java
🔇 Additional comments (2)
src/main/java/org/fungover/breeze/control/RetryExecutor.java (1)
30-45: Use a dedicated logging framework instead of System.out/err.These lines repeatedly use
System.out.println()andSystem.err.println(). Past review comments have already suggested replacing them with a dedicated logging framework (e.g.,Logger). This approach centralizes log management, honors log levels, and avoids mixing application output with console output.src/main/java/org/fungover/breeze/client/ServerClient.java (1)
16-19: ValidatebusyProbabilityrange.Currently, there is no check to ensure
busyProbabilityis between 0 and 1. AbusyProbabilityvalue outside[0, 1]could lead to unexpected behavior inattemptFetchData().
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
src/main/java/org/fungover/breeze/client/ServerClient.java (2)
27-29: Catch narrower exceptions or rethrow for clarity.Currently,
fetchData()can throw anyException, which may complicate error handling in higher-level code. Consider narrowing or rethrowing exceptions to indicate specific failure modes (e.g.,IOException,ServerBusyException, etc.).
31-36: Use descriptive return values or domain objects.Returning a generic
"Sample Data"string might be fine for testing, but in production code, returning a domain-specific object or a well-defined result type can improve clarity and maintainability.src/test/java/org/fungover/breeze/client/ServerClientTest.java (1)
9-12: Expand coverage for different probabilities.Current test covers only
busyProbability=0.0. Add tests for a mid-range probability (e.g. 0.5) to confirm partial server-busy scenarios and the retry mechanism’s behavior.Do you want me to generate stubs for additional test coverage?
src/main/java/org/fungover/breeze/control/RetryExecutor.java (4)
45-47: Replace standard output with a structured logger.Using
System.out.printffor logging can become unwieldy in production. A logging framework offers better configurability, severity levels, and message formatting.
49-60: Consistent error handling strategy.Exceptions that are not assignable to
retryOnescalate asIllegalArgumentException. This can be confusing if the real exception type is not closely related. Consider a more descriptive exception or a separate path for non-retryable errors.
71-73: Make backoff parameters configurable.The
1.5multiplier and500jitter are hard-coded. Exposing them as configurable parameters (e.g., via theBuilder) can improve flexibility and help tune retry behavior.
128-139: Promote reuse of the builder for custom scenarios.
executeWithRetryprovides a convenient default but might encourage usage without deeper configuration. Encourage advanced users to leverage the builder for more nuanced backoff settings and custom exceptions.src/test/java/org/fungover/breeze/control/RetryExecutorTest.java (5)
43-60: Consider enhancing exhaustion handler verification.While the test verifies that the handler is called, it could be improved by also verifying the exception details passed to the handler.
@Test void executeShouldCallOnExhaustionHandler() { AtomicBoolean handlerCalled = new AtomicBoolean(false); + AtomicReference<Throwable> caughtException = new AtomicReference<>(); RetryExecutor executor = RetryExecutor.builder() .maxAttempts(1) .onExhaustion(ex -> { handlerCalled.set(true); + caughtException.set(ex); return true; }) .build(); assertThrows(RetryExecutor.RetryExhaustedException.class, () -> executor.execute(() -> { throw new ServerBusyException(); }) ); assertTrue(handlerCalled.get()); + assertNotNull(caughtException.get()); + assertTrue(caughtException.get() instanceof ServerBusyException); }
62-64: Consider adding edge cases to parameterized test.The test could be more comprehensive by including edge cases such as 0 (immediate success) and values greater than TEST_MAX_ATTEMPTS.
@ParameterizedTest -@ValueSource(ints = {1, 2, 3}) +@ValueSource(ints = {0, 1, 2, 3, 4}) void executeShouldRetryCorrectNumberOfTimes(int failedAttempts) throws Exception {
98-105: Consider verifying specific default configuration values.The test could be more thorough by verifying that specific default values (max attempts, delays, etc.) are being used.
107-115: Make the assertion more precise.Using
containsfor string comparison is less precise than an exact match.- assertTrue(result.contains("Success after 3")); + assertEquals("Success after 3 tries", result);
117-134: Consider making the timing test more robust.Time-based tests can be flaky. Consider:
- Using a clock abstraction for more reliable testing
- Adding some margin of error in the assertion
- Mocking the delay mechanism
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/fungover/breeze/client/ServerClient.java(1 hunks)src/main/java/org/fungover/breeze/control/RetryExecutor.java(1 hunks)src/test/java/org/fungover/breeze/client/ServerClientTest.java(1 hunks)src/test/java/org/fungover/breeze/control/RetryExecutorTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: builds
🔇 Additional comments (6)
src/main/java/org/fungover/breeze/client/ServerClient.java (3)
10-14: Consider thread-safety if multiple threads call this class.Storing a single
Randomfield in a multi-threaded environment can lead to concurrency issues, especially with non-thread-safe random number generators. If you anticipate multi-thread use, considerThreadLocalRandomor synchronizing access to theRandominstance.
15-17: Good use of constructor chaining.Neatly delegates to the parameterized constructor, ensuring consistent initialization.
19-25: Validate random usage outside test contexts.Using
SecureRandomby default can be more costly than standardRandom; verify that the cryptographic strength is actually needed for production. If not, considerRandomorThreadLocalRandomfor performance.src/test/java/org/fungover/breeze/control/RetryExecutorTest.java (3)
13-24: LGTM! Well-structured test class setup.The helper method and constants are well-designed, making the tests more maintainable and readable.
26-31: LGTM! Clear and focused test case.The test effectively verifies the basic success case without retries.
1-135: PR title does not match the changes.The PR title mentions "SIMD Optimization for Vector Classes" but the changes implement retry logic. Please update the PR title to accurately reflect the changes.
| @Test | ||
| void testFailedFetch() { | ||
| ServerClient client = new ServerClient(1.0, new SecureRandom()); | ||
| assertThrows(Exception.class, client::fetchData); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test for exact exception type.
Rather than assertThrows(Exception.class), consider using assertThrows(ServerBusyException.class). This ensures the test checks the correct exception precisely, improving clarity.
-assertThrows(Exception.class, client::fetchData);
+assertThrows(ServerBusyException.class, client::fetchData);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void testFailedFetch() { | |
| ServerClient client = new ServerClient(1.0, new SecureRandom()); | |
| assertThrows(Exception.class, client::fetchData); | |
| } | |
| @Test | |
| void testFailedFetch() { | |
| ServerClient client = new ServerClient(1.0, new SecureRandom()); | |
| assertThrows(ServerBusyException.class, client::fetchData); | |
| } |
| while (attemptCount < maxAttempts) { | ||
| try { | ||
| logAttempt(attemptCount + 1); | ||
| return operation.run(); | ||
| } catch (Exception ex) { | ||
| handleOperationError(ex, attemptCount, currentDelay); | ||
| attemptCount++; | ||
| currentDelay = calculateNextDelay(currentDelay); | ||
| Thread.sleep(currentDelay); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle thread interruption gracefully.
When using Thread.sleep(currentDelay) in a loop, consider catching InterruptedException specifically and either restore the interrupted state or decide whether to break out of retries to avoid unwanted infinite retries in interrupted threads.
try {
Thread.sleep(currentDelay);
} catch (InterruptedException ie) {
+ Thread.currentThread().interrupt();
throw ie;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (attemptCount < maxAttempts) { | |
| try { | |
| logAttempt(attemptCount + 1); | |
| return operation.run(); | |
| } catch (Exception ex) { | |
| handleOperationError(ex, attemptCount, currentDelay); | |
| attemptCount++; | |
| currentDelay = calculateNextDelay(currentDelay); | |
| Thread.sleep(currentDelay); | |
| } | |
| } | |
| while (attemptCount < maxAttempts) { | |
| try { | |
| logAttempt(attemptCount + 1); | |
| return operation.run(); | |
| } catch (Exception ex) { | |
| handleOperationError(ex, attemptCount, currentDelay); | |
| attemptCount++; | |
| currentDelay = calculateNextDelay(currentDelay); | |
| try { | |
| Thread.sleep(currentDelay); | |
| } catch (InterruptedException ie) { | |
| Thread.currentThread().interrupt(); | |
| throw ie; | |
| } | |
| } | |
| } |
| @Test | ||
| void executeShouldNotRetryNonRetryableExceptions() { | ||
| RetryExecutor executor = createDefaultExecutor(ServerBusyException.class); | ||
| assertThrows(IllegalArgumentException.class, () -> | ||
| executor.execute(() -> { | ||
| throw new IOException("Non-retryable error"); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix incorrect exception type in test.
The test is configured to retry on ServerBusyException but throws IOException while asserting IllegalArgumentException. This appears to be incorrect.
@Test
void executeShouldNotRetryNonRetryableExceptions() {
RetryExecutor executor = createDefaultExecutor(ServerBusyException.class);
assertThrows(IllegalArgumentException.class, () ->
executor.execute(() -> {
- throw new IOException("Non-retryable error");
+ throw new IllegalArgumentException("Non-retryable error");
})
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| void executeShouldNotRetryNonRetryableExceptions() { | |
| RetryExecutor executor = createDefaultExecutor(ServerBusyException.class); | |
| assertThrows(IllegalArgumentException.class, () -> | |
| executor.execute(() -> { | |
| throw new IOException("Non-retryable error"); | |
| }) | |
| ); | |
| } | |
| @Test | |
| void executeShouldNotRetryNonRetryableExceptions() { | |
| RetryExecutor executor = createDefaultExecutor(ServerBusyException.class); | |
| assertThrows(IllegalArgumentException.class, () -> | |
| executor.execute(() -> { | |
| throw new IllegalArgumentException("Non-retryable error"); | |
| }) | |
| ); | |
| } |
Stelle83
left a comment
Stelle83
left a comment
There was a problem hiding this comment.
Consider adding more descriptive documentation/comments for better understanding of the methods and their purposes.
Overall, the changes improve code robustness, readability, and maintainability.
| import java.util.function.Function; | ||
|
|
||
| public class RetryExecutor { | ||
| private static final SecureRandom JITTER = new SecureRandom(); |
There was a problem hiding this comment.
Good use of SecureRandom for cryptographic randomness!
Signed-off-by: Patti88 <patricia.nilsson@iths.se>
…ttps://github.com/fungover/breeze into Implement-SIMD-Optimization-for-Vector-Classes-106
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pom.xml (1)
68-78: Ensure compatibility with incubator modules and Java 23.While specifying
<maven.compiler.release>23</maven.compiler.release>and adding--add-modules=jdk.incubator.vectoris valid for the latest JDK preview or incubator features, it may introduce compatibility issues or require a specific JDK build. Please confirm your build environment supports Java 23 and the incubator module.Would you like a script to verify the installed JDK version and confirm that incubator modules are supported in your CI environment?
src/test/java/org/fungover/breeze/vector/Vector4Test.java (1)
49-59: Consider parameterizing repeated test patterns.Several tests (e.g., verifying components after arithmetic, checking null inputs) follow the same structure, differing primarily by operation type. Parameterized tests or a shared helper method could reduce duplication and streamline additions of future vector operations.
Also applies to: 72-86
src/main/java/org/fungover/breeze/vector/Vector4.java (1)
68-73: Eliminate repeated vector species initialization.You repeatedly retrieve
FloatVector.SPECIES_128across several methods. Consider extracting it into a private static final field, e.g.:+ private static final VectorSpecies<Float> SPECIES = FloatVector.SPECIES_128;This reduces duplication and clarifies the chosen vector species.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xml(1 hunks)src/main/java/org/fungover/breeze/vector/Vector4.java(1 hunks)src/test/java/org/fungover/breeze/vector/Vector4Test.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: builds
🔇 Additional comments (3)
pom.xml (1)
79-88: Validate test builds with the new module.Including the same incubator module argument in the Surefire plugin is correct to ensure tests can compile and run. Double-check that your test coverage or other testing frameworks (e.g., Failsafe if you have integration tests) include the same module arguments for consistency throughout your build.
src/test/java/org/fungover/breeze/vector/Vector4Test.java (1)
16-18: Good documentation on the 'add' tests and accurate assertions.These Javadoc comments concisely explain the test intention, and the
assertThat(...).isCloseTo(...)checks are correctly tuned for floating-point comparisons. This clarity is commendable.Also applies to: 26-29
src/main/java/org/fungover/breeze/vector/Vector4.java (1)
123-124: Reassess floating-point threshold for zero check.Using
Math.abs(d) < 1e-6fmay classify very small but non-zero values as zero. If small divisors are legitimate, this threshold could unintentionally raise an exception. Consider making the epsilon configurable or using an exact check when that better aligns with your requirements.
AfagMamedova
left a comment
AfagMamedova
left a comment
There was a problem hiding this comment.
Hej!
Jag tycker det här ser bra ut och det är kul att du försöker göra koden snabbare med SIMD.
Tydlig och snygg kod, lätt att följa. Bra idé att försöka göra beräkningar snabbare.
Kanske lägga till en alternativ lösning för datorer som inte kan köra SIMD?
Några tester vore bra, så man ser att allt fungerar som det ska.
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests