refactor: reduce allocations in the sim task #842
Conversation
| Ok(OrderSimResult::Success( | ||
| Arc::new(SimulatedOrder { | ||
| order, | ||
| order: order.into_owned(), |
There was a problem hiding this comment.
using the Cow allows us to defer the clone to here. This means the clone is eliminated in the (presumably common) case that simulation fails
| pub fn simulate_order_using_fork<Tracer: SimulationTracer>( | ||
| parent_orders: Vec<Order>, | ||
| order: Order, | ||
| parent_orders: &[Order], |
There was a problem hiding this comment.
using the reference allows us to not clone the parents, saving a potentially very large allocation
There was a problem hiding this comment.
Pull request overview
This PR refactors the simulator task to reduce unnecessary cloning and defer allocations for better performance. The changes are primarily focused on passing references instead of owned values and using Cow<'_, Order> to defer cloning until the value needs to be owned.
Key changes:
- Function signatures for
simulate_orderandsimulate_order_using_forknow accept&[Order]for parent orders andCow<'_, Order>for the main order NonceKeystruct changed fromClonetoCopysince it only contains copy types- Removed unnecessary
.clone()calls throughout the codebase
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| simulation_job.rs | Updated comment about tokio::select fairness with documentation link |
| sim_worker.rs | Changed to pass references to parent orders and wrap owned order in Cow::Owned |
| sim.rs | Core refactoring: changed function signatures to accept references, added Cow for deferred cloning, removed unnecessary clones, added tracing instrumentation, and changed NonceKey to Copy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the contribution, James! Most of the team is currently on vacation, and will be around again next week. |
|
Thanks for the work but we are about to change Order for Arc (since Order is something that almost never should be modified) so there is no need for introducing Cow. |
|
Whether the Order is Arced or not, cloning the Vec requires an allocation which should be removed |
## 📝 Summary No longer based on #842, which has been closed Improves and corrects documentation in the live_builder, particularly around Order propagation through the OrderPool. Functionality does not change. Most significant API change is around `BlobTypeOrderFilter`, by moving constructors from free functions to assoc functions - Improve and correct `OrderPool` documentation - Add `Copy` impl for `ReplacementData<KeyType>` - Add missing `Copy` impl for `NonceKey` - make implementation of `BlobTypeOrderFilter` construction idiomatic for Rust - add missing `trace!` event to `BlobTypeOrderFilter` when filtering an order - add `rule_name` to `BlobTypeOrderFilter` to ensure behavior is clear in tracing event - add `From` impls for `ReplaceableOrderEvent`, and remove some relevant `.clone` invocations - update `AutoRemovingOrderPoolSubscriptionId` to use `Weak` to prevent them from holding `OrderPool` memory alloc for dying subscriptions - use `FuturesUnordered` instead of `Vec<JoinHandle<()>>` in the order pool to ensure parallelization of handle reaping ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [ ] Added tests (if applicable)
📝 Summary
Reduce cloning in the simulator task. This minimizes and defers new allocations. It is both code cleanup and perf. There are no logic changes.
✅ I have completed the following steps:
make lintmake test