feat: Object Pooling for Memory Optimization#3
Open
castletrade wants to merge 11 commits into
Open
Conversation
Guía para revisoresIntroduce un pool de objetos Order para reducir la presión del GC, refactoriza el modelo Order para admitir la reutilización mediante constructores y un método clear(), y añade pruebas unitarias que validan el comportamiento del pool y el manejo del agotamiento. Diagrama de secuencia para tomar prestados y devolver Orders a través de OrderPoolsequenceDiagram
actor Client
participant OrderPool
participant Order
Client->>OrderPool: borrowOrder()
alt Order available in pool
OrderPool->>OrderPool: poll()
OrderPool-->>Client: pooled Order
else Pool exhausted
OrderPool->>Order: new Order()
OrderPool-->>Client: transient Order
end
Client->>OrderPool: returnOrder(order)
OrderPool->>Order: clear()
OrderPool->>OrderPool: offer(order)
alt Pool full
OrderPool-->>Client: discard order
else Accepted into pool
OrderPool-->>Client: order returned to pool
end
Diagrama de clases para Order y OrderPool con soporte de pool de objetosclassDiagram
class Order {
String id
String symbol
AssetClass assetClass
OrderSide side
Integer quantity
Double price
ExecutionType executionType
OrderStatus status
LocalDateTime timestamp
+Order()
+Order(String id, String symbol, AssetClass assetClass, OrderSide side, Integer quantity, Double price, ExecutionType executionType, OrderStatus status, LocalDateTime timestamp)
+void clear()
}
class AssetClass {
<<enumeration>>
EQUITY
FX
CRYPTO
FIXED_INCOME
}
class OrderPool {
-BlockingQueue~Order~ pool
-static int DEFAULT_POOL_SIZE
+OrderPool()
+OrderPool(int size)
+Order borrowOrder()
+void returnOrder(Order order)
+int getAvailableCount()
}
Order o-- AssetClass
OrderPool o--> Order
Cambios a nivel de archivo
Posibles issues relacionados
Consejos y comandosInteracción con Sourcery
Personalizar tu experienciaAccede a tu dashboard para:
Obtener ayuda
Original review guide in EnglishReviewer's GuideIntroduces an Order object pool to reduce GC pressure, refactors the Order model to support reuse via constructors and a clear() method, and adds unit tests validating pool behavior and exhaustion handling. Sequence diagram for borrowing and returning Orders via OrderPoolsequenceDiagram
actor Client
participant OrderPool
participant Order
Client->>OrderPool: borrowOrder()
alt Order available in pool
OrderPool->>OrderPool: poll()
OrderPool-->>Client: pooled Order
else Pool exhausted
OrderPool->>Order: new Order()
OrderPool-->>Client: transient Order
end
Client->>OrderPool: returnOrder(order)
OrderPool->>Order: clear()
OrderPool->>OrderPool: offer(order)
alt Pool full
OrderPool-->>Client: discard order
else Accepted into pool
OrderPool-->>Client: order returned to pool
end
Class diagram for Order and OrderPool with object pooling supportclassDiagram
class Order {
String id
String symbol
AssetClass assetClass
OrderSide side
Integer quantity
Double price
ExecutionType executionType
OrderStatus status
LocalDateTime timestamp
+Order()
+Order(String id, String symbol, AssetClass assetClass, OrderSide side, Integer quantity, Double price, ExecutionType executionType, OrderStatus status, LocalDateTime timestamp)
+void clear()
}
class AssetClass {
<<enumeration>>
EQUITY
FX
CRYPTO
FIXED_INCOME
}
class OrderPool {
-BlockingQueue~Order~ pool
-static int DEFAULT_POOL_SIZE
+OrderPool()
+OrderPool(int size)
+Order borrowOrder()
+void returnOrder(Order order)
+int getAvailableCount()
}
Order o-- AssetClass
OrderPool o--> Order
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - he encontrado 7 problemas y he dejado algunos comentarios de alto nivel:
- Actualmente el tamaño del pool está codificado (y el @component solo usa el constructor sin argumentos); plantéate hacer que la capacidad del pool sea configurable externamente (por ejemplo, mediante propiedades de la aplicación / inyección por constructor) para poder ajustarla por despliegue.
- Dado que las instancias de Order ahora son mutables y se reutilizan entre hilos a través del pool, sería útil documentar explícitamente en el Javadoc de OrderPool que los consumidores no deben retener referencias ni usar un Order después de devolverlo al pool para evitar errores de concurrencia sutiles.
- El log de advertencia cuando se agota el pool puede volverse muy ruidoso bajo carga sostenida; considera reducir el nivel de log, aplicar limitación de frecuencia (rate limiting) o registrar el estado de agotamiento solo cuando se cruce un umbral.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pool size is currently hardcoded (and the @Component uses only the no-arg constructor); consider making the pool capacity externally configurable (e.g., via application properties / constructor injection) so it can be tuned per deployment.
- Since Order instances are now mutable and reused across threads via the pool, it would be helpful to explicitly document in the OrderPool Javadoc that callers must not retain references or use an Order after returning it to the pool to avoid subtle concurrency bugs.
- The warning log on pool exhaustion may become very noisy under sustained load; consider either reducing the log level, rate limiting, or logging the exhaustion state only when a threshold is crossed.
## Individual Comments
### Comment 1
<location path="src/main/java/com/castletrade/oms/core/domain/model/Order.java" line_range="29-38" />
<code_context>
+ /**
+ * Resets the order state for reuse in an object pool.
+ */
+ public void clear() {
+ this.id = null;
+ this.symbol = null;
+ this.assetClass = null;
+ this.side = null;
+ this.quantity = null;
+ this.price = null;
+ this.executionType = null;
+ this.status = null;
+ this.timestamp = null;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider restricting `clear()` visibility to limit misuse outside pooling contexts.
As a public method, `clear()` lets any caller reset an `Order` that may still be in use, which can cause subtle bugs (e.g., entries in collections suddenly losing their identity fields). Since it exists for pooling, consider making it package‑private or otherwise limiting visibility so only `OrderPool` can call it.
Suggested implementation:
```java
/**
* Resets the order state for reuse in an object pool.
* Package-private to prevent misuse outside pooling contexts.
*/
void clear() {
this.id = null;
this.symbol = null;
this.assetClass = null;
this.side = null;
this.quantity = null;
this.price = null;
```
1. Ensure any code (e.g., `OrderPool`) that invokes `clear()` resides in the same package `com.castletrade.oms.core.domain.model` or adjust its package accordingly.
2. If there are tests calling `order.clear()` from a different package, either move those tests into the same package or access it via the pooling abstraction instead of directly calling `clear()`.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="31-39" />
<code_context>
+ /**
+ * Borrows an order from the pool.
+ */
+ public Order borrowOrder() {
+ Order order = pool.poll();
+ if (order == null) {
+ log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
+ return new Order();
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Using WARN for pool exhaustion in a hot path may generate excessive log noise under load.
In high-throughput scenarios, this condition may be hit frequently, so `WARN` here can both slow the hot path and flood logs. Consider lowering the level to `INFO`/`DEBUG` or adding throttling (e.g., log every Nth occurrence or on a time interval) to preserve observability without excessive log I/O.
```suggestion
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
```
</issue_to_address>
### Comment 3
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="46-51" />
<code_context>
+ /**
+ * Returns an order to the pool after resetting its state.
+ */
+ public void returnOrder(Order order) {
+ if (order == null) return;
+ order.clear();
+ boolean accepted = pool.offer(order);
+ if (!accepted) {
+ log.debug("OrderPool full, discarding order object.");
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning the same `Order` instance multiple times can lead to multiple references to the same object in the pool.
Since `ArrayBlockingQueue` allows duplicates, the same `Order` can be added to the pool multiple times if `returnOrder` is called twice on it (or on an object never borrowed). That can lead to the same mutable instance being handed to multiple borrowers concurrently. If this is possible in your usage, add a guard (e.g., an `inPool` flag on `Order` or a small tracking structure) to prevent double returns or foreign objects being returned.
</issue_to_address>
### Comment 4
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="9-18" />
<code_context>
+class OrderPoolTest {
+
+ @Test
+ void testBorrowAndReturn() {
+ OrderPool pool = new OrderPool(10);
+ assertEquals(10, pool.getAvailableCount());
+
+ Order order = pool.borrowOrder();
+ assertNotNull(order);
+ assertEquals(9, pool.getAvailableCount());
+
+ order.setSymbol("BTC/USD");
+ pool.returnOrder(order);
+
+ assertEquals(10, pool.getAvailableCount());
+
+ Order reborrowed = pool.borrowOrder();
+ assertNull(reborrowed.getSymbol(), "Order should be cleared when returned to pool");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the borrow/return test to verify all Order fields are cleared, not just symbol.
Since `clear()` resets all fields (`id`, `assetClass`, `side`, `quantity`, `price`, `executionType`, `status`, `timestamp`), initialize several of these before `returnOrder` and assert they are all cleared on re-borrow. This strengthens the test to catch future regressions where a field is accidentally not reset.
Suggested implementation:
```java
@Test
void testBorrowAndReturn() {
OrderPool pool = new OrderPool(10);
assertEquals(10, pool.getAvailableCount());
Order order = pool.borrowOrder();
assertNotNull(order);
assertEquals(9, pool.getAvailableCount());
// initialize multiple fields so we can verify they are cleared
order.setId(123L);
order.setSymbol("BTC/USD");
order.setAssetClass(AssetClass.CRYPTO);
order.setSide(Side.BUY);
order.setQuantity(new java.math.BigDecimal("1.23"));
order.setPrice(new java.math.BigDecimal("45678.90"));
order.setExecutionType(ExecutionType.MARKET);
order.setStatus(OrderStatus.NEW);
order.setTimestamp(java.time.Instant.now());
pool.returnOrder(order);
assertEquals(10, pool.getAvailableCount());
Order reborrowed = pool.borrowOrder();
// all fields should be cleared when returned to pool
assertNull(reborrowed.getId(), "Order id should be cleared when returned to pool");
assertNull(reborrowed.getSymbol(), "Order symbol should be cleared when returned to pool");
assertNull(reborrowed.getAssetClass(), "Order assetClass should be cleared when returned to pool");
assertNull(reborrowed.getSide(), "Order side should be cleared when returned to pool");
assertNull(reborrowed.getQuantity(), "Order quantity should be cleared when returned to pool");
assertNull(reborrowed.getPrice(), "Order price should be cleared when returned to pool");
assertNull(reborrowed.getExecutionType(), "Order executionType should be cleared when returned to pool");
assertNull(reborrowed.getStatus(), "Order status should be cleared when returned to pool");
assertNull(reborrowed.getTimestamp(), "Order timestamp should be cleared when returned to pool");
}
```
- Ensure the `Order` class exposes getters/setters matching the method names used above: `getId`, `setId`, `getAssetClass`, `setAssetClass`, `getSide`, `setSide`, `getQuantity`, `setQuantity`, `getPrice`, `setPrice`, `getExecutionType`, `setExecutionType`, `getStatus`, `setStatus`, `getTimestamp`, `setTimestamp`.
- Adjust enum/type names if your domain model uses different ones (e.g. `OrderAssetClass` instead of `AssetClass`, `OrderSide` instead of `Side`, `ExecutionType`/`OrderExecutionType`, `OrderStatus` instead of `OrderStatus`, and `Instant` vs another timestamp type).
- If any of these fields are primitives rather than nullable reference types, change the corresponding assertions to check for the appropriate reset default (e.g. `assertEquals(0L, reborrowed.getId())`) instead of `assertNull`.
- You may also want to add static imports for `BigDecimal` and `Instant` (or your timestamp type) instead of using fully-qualified names if that better matches your test code style.
</issue_to_address>
### Comment 5
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="8-17" />
<code_context>
+
+class OrderPoolTest {
+
+ @Test
+ void testBorrowAndReturn() {
+ OrderPool pool = new OrderPool(10);
+ assertEquals(10, pool.getAvailableCount());
+
+ Order order = pool.borrowOrder();
+ assertNotNull(order);
+ assertEquals(9, pool.getAvailableCount());
+
+ order.setSymbol("BTC/USD");
+ pool.returnOrder(order);
+
+ assertEquals(10, pool.getAvailableCount());
+
+ Order reborrowed = pool.borrowOrder();
+ assertNull(reborrowed.getSymbol(), "Order should be cleared when returned to pool");
+ }
+
+ @Test
+ void testPoolExhaustion() {
+ OrderPool pool = new OrderPool(1);
+ pool.borrowOrder();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering `returnOrder(null)` to ensure it is a no-op and doesn’t affect pool size.
There’s logic for `order == null` in `returnOrder`, but no test covering it. Please add a test (e.g. `testReturnNullOrderIsNoOp`) that records `getAvailableCount()`, calls `returnOrder(null)`, and asserts the count is unchanged to guard against regressions in this path.
</issue_to_address>
### Comment 6
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="6" />
<code_context>
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
+class OrderPoolTest {
+
+ @Test
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a focused test for the `Order.clear()` contract, separate from pooling behavior.
Since `Order.clear()` underpins the pooling strategy, a dedicated unit test (e.g., in `OrderTest`) that fully populates an `Order`, calls `clear()`, and asserts every field is reset would help localize failures (Order vs. OrderPool) and reduce the chance of regressions that pool tests don’t catch.
Suggested implementation:
```java
order.setSymbol("BTC/USD");
pool.returnOrder(order);
assertEquals(10, pool.getAvailableCount());
}
}
class OrderTest {
@Test
void clear_resets_all_fields() {
Order order = new Order();
order.setSymbol("BTC/USD");
order.clear();
assertNull(order.getSymbol());
}
}
```
To fully align with your review comment ("assert every field is reset"), extend `clear_resets_all_fields` to:
1. Populate every mutable field on `Order` (e.g., quantity, side, price, timestamps, flags, IDs) using whatever setters or builder methods `Order` exposes.
2. After `order.clear()`, assert the post-conditions for each field according to the `clear()` contract (e.g., null for references, 0 for numerics, false for booleans, default enums).
3. If `Order` has derived or cached fields, ensure they are also verified in this test so that regressions in `clear()` are caught independently of pooling behavior.
</issue_to_address>
### Comment 7
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="15" />
<code_context>
+ */
+@Slf4j
+@Component
+public class OrderPool {
+ private final BlockingQueue<Order> pool;
+ private static final int DEFAULT_POOL_SIZE = 1000;
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing an OrderProvider interface and simple alternative implementation so that pooling is hidden behind an abstraction and can be swapped out without touching calling code.
You can keep the pooling optimization but decouple it from the domain and make it easier to swap out with a simpler implementation.
### 1. Introduce an `OrderProvider` interface
Define a minimal abstraction that callers depend on instead of `OrderPool` directly:
```java
package com.castletrade.oms.core.domain.model;
public interface OrderProvider {
Order borrowOrder();
void returnOrder(Order order);
}
```
Then adapt `OrderPool` to implement this:
```java
@Slf4j
@Component
public class OrderPool implements OrderProvider {
// existing implementation unchanged
}
```
All current consumers should be updated to depend on `OrderProvider` instead of `OrderPool`:
```java
// before
private final OrderPool orderPool;
// after
private final OrderProvider orderProvider;
```
```java
// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);
// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);
```
### 2. Add a trivial, non-pooled implementation
You can provide a simple, allocation-only implementation for cases where pooling isn’t needed (or for tests), without touching existing logic:
```java
@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {
@Override
public Order borrowOrder() {
return new Order();
}
@Override
public void returnOrder(Order order) {
// nothing to do
}
}
```
Then `OrderPool` can be used only where really needed, or enabled via configuration/profile:
```java
@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
// current implementation
}
```
This keeps all current `OrderPool` behavior intact but:
- Moves complexity behind a narrow interface.
- Lets you keep the domain model focused on `Order` rather than pooling details.
- Allows you to simplify or even remove pooling later without touching call sites.
</issue_to_address>Sourcery es gratis para proyectos open source - si te gustan nuestras revisiones por favor considera compartirlas ✨
Original comment in English
Hey - I've found 7 issues, and left some high level feedback:
- The pool size is currently hardcoded (and the @component uses only the no-arg constructor); consider making the pool capacity externally configurable (e.g., via application properties / constructor injection) so it can be tuned per deployment.
- Since Order instances are now mutable and reused across threads via the pool, it would be helpful to explicitly document in the OrderPool Javadoc that callers must not retain references or use an Order after returning it to the pool to avoid subtle concurrency bugs.
- The warning log on pool exhaustion may become very noisy under sustained load; consider either reducing the log level, rate limiting, or logging the exhaustion state only when a threshold is crossed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pool size is currently hardcoded (and the @Component uses only the no-arg constructor); consider making the pool capacity externally configurable (e.g., via application properties / constructor injection) so it can be tuned per deployment.
- Since Order instances are now mutable and reused across threads via the pool, it would be helpful to explicitly document in the OrderPool Javadoc that callers must not retain references or use an Order after returning it to the pool to avoid subtle concurrency bugs.
- The warning log on pool exhaustion may become very noisy under sustained load; consider either reducing the log level, rate limiting, or logging the exhaustion state only when a threshold is crossed.
## Individual Comments
### Comment 1
<location path="src/main/java/com/castletrade/oms/core/domain/model/Order.java" line_range="29-38" />
<code_context>
+ /**
+ * Resets the order state for reuse in an object pool.
+ */
+ public void clear() {
+ this.id = null;
+ this.symbol = null;
+ this.assetClass = null;
+ this.side = null;
+ this.quantity = null;
+ this.price = null;
+ this.executionType = null;
+ this.status = null;
+ this.timestamp = null;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider restricting `clear()` visibility to limit misuse outside pooling contexts.
As a public method, `clear()` lets any caller reset an `Order` that may still be in use, which can cause subtle bugs (e.g., entries in collections suddenly losing their identity fields). Since it exists for pooling, consider making it package‑private or otherwise limiting visibility so only `OrderPool` can call it.
Suggested implementation:
```java
/**
* Resets the order state for reuse in an object pool.
* Package-private to prevent misuse outside pooling contexts.
*/
void clear() {
this.id = null;
this.symbol = null;
this.assetClass = null;
this.side = null;
this.quantity = null;
this.price = null;
```
1. Ensure any code (e.g., `OrderPool`) that invokes `clear()` resides in the same package `com.castletrade.oms.core.domain.model` or adjust its package accordingly.
2. If there are tests calling `order.clear()` from a different package, either move those tests into the same package or access it via the pooling abstraction instead of directly calling `clear()`.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="31-39" />
<code_context>
+ /**
+ * Borrows an order from the pool.
+ */
+ public Order borrowOrder() {
+ Order order = pool.poll();
+ if (order == null) {
+ log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
+ return new Order();
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Using WARN for pool exhaustion in a hot path may generate excessive log noise under load.
In high-throughput scenarios, this condition may be hit frequently, so `WARN` here can both slow the hot path and flood logs. Consider lowering the level to `INFO`/`DEBUG` or adding throttling (e.g., log every Nth occurrence or on a time interval) to preserve observability without excessive log I/O.
```suggestion
/**
* Borrows an order from the pool.
*/
public Order borrowOrder() {
Order order = pool.poll();
if (order == null) {
log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size.");
return new Order();
}
```
</issue_to_address>
### Comment 3
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="46-51" />
<code_context>
+ /**
+ * Returns an order to the pool after resetting its state.
+ */
+ public void returnOrder(Order order) {
+ if (order == null) return;
+ order.clear();
+ boolean accepted = pool.offer(order);
+ if (!accepted) {
+ log.debug("OrderPool full, discarding order object.");
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning the same `Order` instance multiple times can lead to multiple references to the same object in the pool.
Since `ArrayBlockingQueue` allows duplicates, the same `Order` can be added to the pool multiple times if `returnOrder` is called twice on it (or on an object never borrowed). That can lead to the same mutable instance being handed to multiple borrowers concurrently. If this is possible in your usage, add a guard (e.g., an `inPool` flag on `Order` or a small tracking structure) to prevent double returns or foreign objects being returned.
</issue_to_address>
### Comment 4
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="9-18" />
<code_context>
+class OrderPoolTest {
+
+ @Test
+ void testBorrowAndReturn() {
+ OrderPool pool = new OrderPool(10);
+ assertEquals(10, pool.getAvailableCount());
+
+ Order order = pool.borrowOrder();
+ assertNotNull(order);
+ assertEquals(9, pool.getAvailableCount());
+
+ order.setSymbol("BTC/USD");
+ pool.returnOrder(order);
+
+ assertEquals(10, pool.getAvailableCount());
+
+ Order reborrowed = pool.borrowOrder();
+ assertNull(reborrowed.getSymbol(), "Order should be cleared when returned to pool");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the borrow/return test to verify all Order fields are cleared, not just symbol.
Since `clear()` resets all fields (`id`, `assetClass`, `side`, `quantity`, `price`, `executionType`, `status`, `timestamp`), initialize several of these before `returnOrder` and assert they are all cleared on re-borrow. This strengthens the test to catch future regressions where a field is accidentally not reset.
Suggested implementation:
```java
@Test
void testBorrowAndReturn() {
OrderPool pool = new OrderPool(10);
assertEquals(10, pool.getAvailableCount());
Order order = pool.borrowOrder();
assertNotNull(order);
assertEquals(9, pool.getAvailableCount());
// initialize multiple fields so we can verify they are cleared
order.setId(123L);
order.setSymbol("BTC/USD");
order.setAssetClass(AssetClass.CRYPTO);
order.setSide(Side.BUY);
order.setQuantity(new java.math.BigDecimal("1.23"));
order.setPrice(new java.math.BigDecimal("45678.90"));
order.setExecutionType(ExecutionType.MARKET);
order.setStatus(OrderStatus.NEW);
order.setTimestamp(java.time.Instant.now());
pool.returnOrder(order);
assertEquals(10, pool.getAvailableCount());
Order reborrowed = pool.borrowOrder();
// all fields should be cleared when returned to pool
assertNull(reborrowed.getId(), "Order id should be cleared when returned to pool");
assertNull(reborrowed.getSymbol(), "Order symbol should be cleared when returned to pool");
assertNull(reborrowed.getAssetClass(), "Order assetClass should be cleared when returned to pool");
assertNull(reborrowed.getSide(), "Order side should be cleared when returned to pool");
assertNull(reborrowed.getQuantity(), "Order quantity should be cleared when returned to pool");
assertNull(reborrowed.getPrice(), "Order price should be cleared when returned to pool");
assertNull(reborrowed.getExecutionType(), "Order executionType should be cleared when returned to pool");
assertNull(reborrowed.getStatus(), "Order status should be cleared when returned to pool");
assertNull(reborrowed.getTimestamp(), "Order timestamp should be cleared when returned to pool");
}
```
- Ensure the `Order` class exposes getters/setters matching the method names used above: `getId`, `setId`, `getAssetClass`, `setAssetClass`, `getSide`, `setSide`, `getQuantity`, `setQuantity`, `getPrice`, `setPrice`, `getExecutionType`, `setExecutionType`, `getStatus`, `setStatus`, `getTimestamp`, `setTimestamp`.
- Adjust enum/type names if your domain model uses different ones (e.g. `OrderAssetClass` instead of `AssetClass`, `OrderSide` instead of `Side`, `ExecutionType`/`OrderExecutionType`, `OrderStatus` instead of `OrderStatus`, and `Instant` vs another timestamp type).
- If any of these fields are primitives rather than nullable reference types, change the corresponding assertions to check for the appropriate reset default (e.g. `assertEquals(0L, reborrowed.getId())`) instead of `assertNull`.
- You may also want to add static imports for `BigDecimal` and `Instant` (or your timestamp type) instead of using fully-qualified names if that better matches your test code style.
</issue_to_address>
### Comment 5
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="8-17" />
<code_context>
+
+class OrderPoolTest {
+
+ @Test
+ void testBorrowAndReturn() {
+ OrderPool pool = new OrderPool(10);
+ assertEquals(10, pool.getAvailableCount());
+
+ Order order = pool.borrowOrder();
+ assertNotNull(order);
+ assertEquals(9, pool.getAvailableCount());
+
+ order.setSymbol("BTC/USD");
+ pool.returnOrder(order);
+
+ assertEquals(10, pool.getAvailableCount());
+
+ Order reborrowed = pool.borrowOrder();
+ assertNull(reborrowed.getSymbol(), "Order should be cleared when returned to pool");
+ }
+
+ @Test
+ void testPoolExhaustion() {
+ OrderPool pool = new OrderPool(1);
+ pool.borrowOrder();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering `returnOrder(null)` to ensure it is a no-op and doesn’t affect pool size.
There’s logic for `order == null` in `returnOrder`, but no test covering it. Please add a test (e.g. `testReturnNullOrderIsNoOp`) that records `getAvailableCount()`, calls `returnOrder(null)`, and asserts the count is unchanged to guard against regressions in this path.
</issue_to_address>
### Comment 6
<location path="src/test/java/com/castletrade/oms/core/domain/model/OrderPoolTest.java" line_range="6" />
<code_context>
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
+class OrderPoolTest {
+
+ @Test
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a focused test for the `Order.clear()` contract, separate from pooling behavior.
Since `Order.clear()` underpins the pooling strategy, a dedicated unit test (e.g., in `OrderTest`) that fully populates an `Order`, calls `clear()`, and asserts every field is reset would help localize failures (Order vs. OrderPool) and reduce the chance of regressions that pool tests don’t catch.
Suggested implementation:
```java
order.setSymbol("BTC/USD");
pool.returnOrder(order);
assertEquals(10, pool.getAvailableCount());
}
}
class OrderTest {
@Test
void clear_resets_all_fields() {
Order order = new Order();
order.setSymbol("BTC/USD");
order.clear();
assertNull(order.getSymbol());
}
}
```
To fully align with your review comment ("assert every field is reset"), extend `clear_resets_all_fields` to:
1. Populate every mutable field on `Order` (e.g., quantity, side, price, timestamps, flags, IDs) using whatever setters or builder methods `Order` exposes.
2. After `order.clear()`, assert the post-conditions for each field according to the `clear()` contract (e.g., null for references, 0 for numerics, false for booleans, default enums).
3. If `Order` has derived or cached fields, ensure they are also verified in this test so that regressions in `clear()` are caught independently of pooling behavior.
</issue_to_address>
### Comment 7
<location path="src/main/java/com/castletrade/oms/core/domain/model/OrderPool.java" line_range="15" />
<code_context>
+ */
+@Slf4j
+@Component
+public class OrderPool {
+ private final BlockingQueue<Order> pool;
+ private static final int DEFAULT_POOL_SIZE = 1000;
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing an OrderProvider interface and simple alternative implementation so that pooling is hidden behind an abstraction and can be swapped out without touching calling code.
You can keep the pooling optimization but decouple it from the domain and make it easier to swap out with a simpler implementation.
### 1. Introduce an `OrderProvider` interface
Define a minimal abstraction that callers depend on instead of `OrderPool` directly:
```java
package com.castletrade.oms.core.domain.model;
public interface OrderProvider {
Order borrowOrder();
void returnOrder(Order order);
}
```
Then adapt `OrderPool` to implement this:
```java
@Slf4j
@Component
public class OrderPool implements OrderProvider {
// existing implementation unchanged
}
```
All current consumers should be updated to depend on `OrderProvider` instead of `OrderPool`:
```java
// before
private final OrderPool orderPool;
// after
private final OrderProvider orderProvider;
```
```java
// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);
// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);
```
### 2. Add a trivial, non-pooled implementation
You can provide a simple, allocation-only implementation for cases where pooling isn’t needed (or for tests), without touching existing logic:
```java
@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {
@Override
public Order borrowOrder() {
return new Order();
}
@Override
public void returnOrder(Order order) {
// nothing to do
}
}
```
Then `OrderPool` can be used only where really needed, or enabled via configuration/profile:
```java
@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
// current implementation
}
```
This keeps all current `OrderPool` behavior intact but:
- Moves complexity behind a narrow interface.
- Lets you keep the domain model focused on `Order` rather than pooling details.
- Allows you to simplify or even remove pooling later without touching call sites.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+29
to
+38
| public void clear() { | ||
| this.id = null; | ||
| this.symbol = null; | ||
| this.assetClass = null; | ||
| this.side = null; | ||
| this.quantity = null; | ||
| this.price = null; | ||
| this.executionType = null; | ||
| this.status = null; | ||
| this.timestamp = null; |
There was a problem hiding this comment.
suggestion (bug_risk): Considera restringir la visibilidad de clear() para limitar su mal uso fuera de los contextos de pool.
Como método público, clear() permite que cualquier consumidor reinicie un Order que puede seguir en uso, lo que puede causar errores sutiles (por ejemplo, entradas en colecciones que de repente pierden sus campos de identidad). Dado que existe para el uso con el pool, considera hacerlo con visibilidad de paquete (package‑private) o limitar su visibilidad de otra forma de modo que solo OrderPool pueda llamarlo.
Implementación sugerida:
/**
* Resets the order state for reuse in an object pool.
* Package-private to prevent misuse outside pooling contexts.
*/
void clear() {
this.id = null;
this.symbol = null;
this.assetClass = null;
this.side = null;
this.quantity = null;
this.price = null;- Asegúrate de que cualquier código (por ejemplo,
OrderPool) que invoqueclear()resida en el mismo paquetecom.castletrade.oms.core.domain.modelo ajusta su paquete en consecuencia. - Si hay tests que llaman a
order.clear()desde un paquete diferente, mueve esos tests al mismo paquete o accede a través de la abstracción de pooling en lugar de llamar aclear()directamente.
Original comment in English
suggestion (bug_risk): Consider restricting clear() visibility to limit misuse outside pooling contexts.
As a public method, clear() lets any caller reset an Order that may still be in use, which can cause subtle bugs (e.g., entries in collections suddenly losing their identity fields). Since it exists for pooling, consider making it package‑private or otherwise limiting visibility so only OrderPool can call it.
Suggested implementation:
/**
* Resets the order state for reuse in an object pool.
* Package-private to prevent misuse outside pooling contexts.
*/
void clear() {
this.id = null;
this.symbol = null;
this.assetClass = null;
this.side = null;
this.quantity = null;
this.price = null;- Ensure any code (e.g.,
OrderPool) that invokesclear()resides in the same packagecom.castletrade.oms.core.domain.modelor adjust its package accordingly. - If there are tests calling
order.clear()from a different package, either move those tests into the same package or access it via the pooling abstraction instead of directly callingclear().
Comment on lines
+31
to
+39
| /** | ||
| * Borrows an order from the pool. | ||
| */ | ||
| public Order borrowOrder() { | ||
| Order order = pool.poll(); | ||
| if (order == null) { | ||
| log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size."); | ||
| return new Order(); | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Usar WARN para el agotamiento del pool en una ruta caliente puede generar un exceso de ruido en los logs bajo carga.
En escenarios de alto rendimiento, esta condición puede producirse con frecuencia, así que WARN aquí puede tanto ralentizar la ruta caliente como inundar los logs. Considera bajar el nivel a INFO/DEBUG o añadir limitación de frecuencia (por ejemplo, registrar cada N ocurrencias o en un intervalo de tiempo) para mantener la observabilidad sin un E/S de logs excesivo.
Suggested change
| /** | |
| * Borrows an order from the pool. | |
| */ | |
| public Order borrowOrder() { | |
| Order order = pool.poll(); | |
| if (order == null) { | |
| log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size."); | |
| return new Order(); | |
| } | |
| /** | |
| * Borrows an order from the pool. | |
| */ | |
| public Order borrowOrder() { | |
| Order order = pool.poll(); | |
| if (order == null) { | |
| log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size."); | |
| return new Order(); | |
| } |
Original comment in English
suggestion (performance): Using WARN for pool exhaustion in a hot path may generate excessive log noise under load.
In high-throughput scenarios, this condition may be hit frequently, so WARN here can both slow the hot path and flood logs. Consider lowering the level to INFO/DEBUG or adding throttling (e.g., log every Nth occurrence or on a time interval) to preserve observability without excessive log I/O.
Suggested change
| /** | |
| * Borrows an order from the pool. | |
| */ | |
| public Order borrowOrder() { | |
| Order order = pool.poll(); | |
| if (order == null) { | |
| log.warn("OrderPool exhausted, creating new transient object. Consider increasing pool size."); | |
| return new Order(); | |
| } | |
| /** | |
| * Borrows an order from the pool. | |
| */ | |
| public Order borrowOrder() { | |
| Order order = pool.poll(); | |
| if (order == null) { | |
| log.debug("OrderPool exhausted, creating new transient object. Consider increasing pool size."); | |
| return new Order(); | |
| } |
Comment on lines
+46
to
+51
| public void returnOrder(Order order) { | ||
| if (order == null) return; | ||
| order.clear(); | ||
| boolean accepted = pool.offer(order); | ||
| if (!accepted) { | ||
| log.debug("OrderPool full, discarding order object."); |
There was a problem hiding this comment.
issue (bug_risk): Devolver la misma instancia de Order varias veces puede llevar a múltiples referencias al mismo objeto en el pool.
Dado que ArrayBlockingQueue permite duplicados, el mismo Order puede añadirse al pool varias veces si se llama a returnOrder dos veces sobre él (o sobre un objeto que nunca se pidió prestado). Eso puede llevar a que la misma instancia mutable se entregue a múltiples consumidores simultáneamente. Si esto es posible en tu caso de uso, añade una protección (por ejemplo, un flag inPool en Order o una pequeña estructura de seguimiento) para evitar devoluciones dobles u objetos externos que se devuelvan.
Original comment in English
issue (bug_risk): Returning the same Order instance multiple times can lead to multiple references to the same object in the pool.
Since ArrayBlockingQueue allows duplicates, the same Order can be added to the pool multiple times if returnOrder is called twice on it (or on an object never borrowed). That can lead to the same mutable instance being handed to multiple borrowers concurrently. If this is possible in your usage, add a guard (e.g., an inPool flag on Order or a small tracking structure) to prevent double returns or foreign objects being returned.
| */ | ||
| @Slf4j | ||
| @Component | ||
| public class OrderPool { |
There was a problem hiding this comment.
issue (complexity): Considera introducir una interfaz OrderProvider y una implementación alternativa sencilla para que el pooling quede oculto tras una abstracción y pueda sustituirse sin tocar el código consumidor.
Puedes mantener la optimización de pooling pero desacoplarla del dominio y facilitar su sustitución por una implementación más simple.
1. Introduce una interfaz OrderProvider
Define una abstracción mínima de la que dependan los consumidores en lugar de depender directamente de OrderPool:
package com.castletrade.oms.core.domain.model;
public interface OrderProvider {
Order borrowOrder();
void returnOrder(Order order);
}Después, adapta OrderPool para que la implemente:
@Slf4j
@Component
public class OrderPool implements OrderProvider {
// existing implementation unchanged
}Todos los consumidores actuales deberían actualizarse para depender de OrderProvider en lugar de OrderPool:
// before
private final OrderPool orderPool;
// after
private final OrderProvider orderProvider;// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);
// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);2. Añade una implementación trivial sin pool
Puedes proporcionar una implementación sencilla, basada solo en asignaciones, para casos en los que el pooling no sea necesario (o para tests), sin tocar la lógica existente:
@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {
@Override
public Order borrowOrder() {
return new Order();
}
@Override
public void returnOrder(Order order) {
// nothing to do
}
}Después, OrderPool puede usarse solo donde realmente se necesite, o habilitarse mediante configuración/perfil:
@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
// current implementation
}Esto mantiene intacto todo el comportamiento actual de OrderPool, pero:
- Mueve la complejidad detrás de una interfaz estrecha.
- Te permite mantener el modelo de dominio centrado en
Orderen lugar de en detalles de pooling. - Permite simplificar o incluso eliminar el pooling más adelante sin tocar los puntos de uso.
Original comment in English
issue (complexity): Consider introducing an OrderProvider interface and simple alternative implementation so that pooling is hidden behind an abstraction and can be swapped out without touching calling code.
You can keep the pooling optimization but decouple it from the domain and make it easier to swap out with a simpler implementation.
1. Introduce an OrderProvider interface
Define a minimal abstraction that callers depend on instead of OrderPool directly:
package com.castletrade.oms.core.domain.model;
public interface OrderProvider {
Order borrowOrder();
void returnOrder(Order order);
}Then adapt OrderPool to implement this:
@Slf4j
@Component
public class OrderPool implements OrderProvider {
// existing implementation unchanged
}All current consumers should be updated to depend on OrderProvider instead of OrderPool:
// before
private final OrderPool orderPool;
// after
private final OrderProvider orderProvider;// before
Order order = orderPool.borrowOrder();
orderPool.returnOrder(order);
// after
Order order = orderProvider.borrowOrder();
orderProvider.returnOrder(order);2. Add a trivial, non-pooled implementation
You can provide a simple, allocation-only implementation for cases where pooling isn’t needed (or for tests), without touching existing logic:
@Component
@Primary // if you want this as the default
public class SimpleOrderProvider implements OrderProvider {
@Override
public Order borrowOrder() {
return new Order();
}
@Override
public void returnOrder(Order order) {
// nothing to do
}
}Then OrderPool can be used only where really needed, or enabled via configuration/profile:
@Component
@Profile("pooled-orders")
public class OrderPool implements OrderProvider {
// current implementation
}This keeps all current OrderPool behavior intact but:
- Moves complexity behind a narrow interface.
- Lets you keep the domain model focused on
Orderrather than pooling details. - Allows you to simplify or even remove pooling later without touching call sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements an
OrderPooland refactors theOrdermodel to support object reuse.Why:
In high-frequency trading (HFT) environments, frequent allocation/deallocation of short-lived domain objects like Orders can lead to significant Garbage Collection (GC) pauses.
Changes:
@NoArgsConstructorandclear()method toOrderdomain model.OrderPoolusingArrayBlockingQueuefor thread-safe pre-allocation.OrderPoolTest.This closes the Memory Optimization issue.
Summary by Sourcery
Introducir un pool de objetos
Orderpara reducir la sobrecarga de asignación y permitir la reutilización de objetos en cargas de trabajo de trading de alta frecuencia.Nuevas funcionalidades:
OrderPoolque preasigna y gestiona instancias reutilizables deOrdercon un tamaño de pool configurable.Orderdisponibles en el pool.Mejoras:
Ordercon constructores sin argumentos y con todos los argumentos, y un métodoclear()para restablecer el estado para su reutilización.Tests:
OrderPoolTestpara verificar el comportamiento de préstamo, devolución y agotamiento del pool deOrder.Original summary in English
Summary by Sourcery
Introduce an Order object pool to reduce allocation overhead and support object reuse in high-frequency trading workloads.
New Features:
Enhancements:
Tests: