wrap RawAuctionData in Arc for efficient sharing#4392
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Arc wrapping for RawAuctionData to improve data sharing efficiency within the autopilot service. Feedback suggests avoiding the use of &Arc as a function parameter to reduce unnecessary indirection and simplify the API. Additionally, it was noted that deep clones of auction collections are still being performed in the run loop, and refactoring the domain::Auction struct to use Arc for its internal fields would better achieve the intended performance gains.
| orders: auction.orders.clone(), | ||
| prices: auction.prices.clone(), | ||
| surplus_capturing_jit_order_owners: auction.surplus_capturing_jit_order_owners.clone(), |
There was a problem hiding this comment.
These lines perform deep clones of the orders, prices, and surplus_capturing_jit_order_owners collections on every auction cycle. While this is necessary because domain::Auction expects owned data, it significantly limits the performance gains of wrapping RawAuctionData in an Arc. To fully achieve the goal of efficient sharing, consider refactoring the domain::Auction struct to use Arc for its collection fields, allowing these to be cheap pointer increments instead of deep copies.
|
Hey @MartinquaXD, thanks for the input on the last PR. I’d like to learn the proper way to approach these kinds of optimizations going forward. Could you point me toward the usual steps/process you’d expect here, like how to benchmark it properly and validate that the change is actually meaningful? |
Sadly, we need to measure this in production, meaning that you can't really provide the benchmarks out of the box. But in general, we need to measure them before and after, all that not before ensuring they're correct |
Thanks @jmg-duarte , that clears things up. I'll make sure to frame future optimization PRs more carefully and justify claims properly. |
|
The deep clone appears inevitable currently, but it can be delayed until the end. Try applying this diff to your PR. Diffdiff --git crates/autopilot/src/infra/persistence/dto/auction.rs crates/autopilot/src/infra/persistence/dto/auction.rs
index fd75b6d84..6e9aa5393 100644
--- crates/autopilot/src/infra/persistence/dto/auction.rs
+++ crates/autopilot/src/infra/persistence/dto/auction.rs
@@ -22,11 +22,7 @@ pub fn from_domain(auction: &domain::RawAuctionData) -> RawAuctionData {
.iter()
.map(|(key, value)| (**key, value.get().0))
.collect(),
- surplus_capturing_jit_order_owners: auction
- .surplus_capturing_jit_order_owners
- .iter()
- .copied()
- .collect(),
+ surplus_capturing_jit_order_owners: auction.surplus_capturing_jit_order_owners.to_vec(),
}
}
diff --git crates/autopilot/src/infra/persistence/mod.rs crates/autopilot/src/infra/persistence/mod.rs
index 3838e2a09..b1b90b19c 100644
--- crates/autopilot/src/infra/persistence/mod.rs
+++ crates/autopilot/src/infra/persistence/mod.rs
@@ -179,7 +179,7 @@ impl Persistence {
return;
};
tokio::task::spawn(async move {
- let auction_dto = dto::auction::from_domain(&auction);
+ let auction_dto = dto::auction::from_domain(auction.as_ref());
match s3.upload(id.to_string(), auction_dto).await {
Ok(key) => tracing::info!(?key, "uploaded auction to s3"),
Err(err) => tracing::warn!(?err, "failed to upload auction to s3"),
diff --git crates/autopilot/src/run_loop.rs crates/autopilot/src/run_loop.rs
index c47309077..338a0c6e4 100644
--- crates/autopilot/src/run_loop.rs
+++ crates/autopilot/src/run_loop.rs
@@ -88,6 +88,12 @@ pub struct Probes {
pub startup: Arc<Option<AtomicBool>>,
}
+#[derive(Debug)]
+struct CutAuction {
+ id: Id,
+ auction: Arc<domain::RawAuctionData>,
+}
+
pub struct RunLoop {
config: Config,
eth: infra::Ethereum,
@@ -141,7 +147,7 @@ impl RunLoop {
}
pub async fn run_forever(self, mut control: ShutdownController) {
- let mut last_auction = None;
+ let mut last_auction: Option<Arc<domain::RawAuctionData>> = None;
let mut last_block = None;
let self_arc = Arc::new(self);
@@ -255,28 +261,35 @@ impl RunLoop {
async fn next_auction(
&self,
start_block: BlockInfo,
- prev_auction: &mut Option<domain::Auction>,
+ prev_auction: &mut Option<Arc<domain::RawAuctionData>>,
prev_block: &mut Option<B256>,
) -> Option<domain::Auction> {
// wait for appropriate time to start building the auction
- let auction = self.cut_auction().await?;
- tracing::trace!(auction_id = ?auction.id, "auction cut");
+ let CutAuction { id, auction } = self.cut_auction().await?;
+ tracing::trace!(auction_id = ?id, "auction cut");
// Only run the solvers if the auction or block has changed.
- let previous = prev_auction.replace(auction.clone());
- if previous.as_ref() == Some(&auction)
+ let previous = prev_auction.replace(Arc::clone(&auction));
+ if previous.as_deref() == Some(auction.as_ref())
&& prev_block.replace(start_block.hash) == Some(start_block.hash)
{
return None;
}
- observe::log_auction_delta(&previous, &auction, &start_block);
+ observe::log_raw_auction_delta(id, previous.as_deref(), auction.as_ref(), &start_block);
self.probes.liveness.auction();
Metrics::auction_ready(start_block.observed_at);
- Some(auction)
+
+ Some(domain::Auction {
+ id,
+ block: auction.block,
+ orders: auction.orders.clone(),
+ prices: auction.prices.clone(),
+ surplus_capturing_jit_order_owners: auction.surplus_capturing_jit_order_owners.clone(),
+ })
}
- async fn cut_auction(&self) -> Option<domain::Auction> {
+ async fn cut_auction(&self) -> Option<CutAuction> {
let Some(auction) = self.solvable_orders_cache.current_auction().await else {
tracing::debug!("no current auction");
return None;
@@ -290,7 +303,8 @@ impl RunLoop {
Metrics::auction(id);
// always update the auction because the tests use this as a readiness probe
- self.persistence.replace_current_auction_in_db(id, &auction);
+ self.persistence
+ .replace_current_auction_in_db(id, auction.as_ref());
self.persistence
.upload_auction_to_s3(id, Arc::clone(&auction));
@@ -300,13 +314,8 @@ impl RunLoop {
tracing::debug!("skipping empty auction");
return None;
}
- Some(domain::Auction {
- id,
- block: auction.block,
- orders: auction.orders.clone(),
- prices: auction.prices.clone(),
- surplus_capturing_jit_order_owners: auction.surplus_capturing_jit_order_owners.clone(),
- })
+
+ Some(CutAuction { id, auction })
}
#[instrument(skip_all)]
@@ -1085,37 +1094,61 @@ pub mod observe {
std::collections::HashSet,
};
- pub fn log_auction_delta(
- previous: &Option<domain::Auction>,
- current: &domain::Auction,
+ fn log_order_delta<I, J>(
+ id: domain::auction::Id,
+ previous: I,
+ current: J,
start_block: &BlockInfo,
- ) {
- let previous_uids = match previous {
- Some(previous) => previous
- .orders
- .iter()
- .map(|order| order.uid)
- .collect::<HashSet<_>>(),
- None => HashSet::new(),
- };
- let current_uids = current
- .orders
- .iter()
- .map(|order| order.uid)
- .collect::<HashSet<_>>();
+ ) where
+ I: IntoIterator<Item = domain::OrderUid>,
+ J: IntoIterator<Item = domain::OrderUid>,
+ {
+ let previous_uids = previous.into_iter().collect::<HashSet<_>>();
+ let current_uids = current.into_iter().collect::<HashSet<_>>();
let added = current_uids.difference(&previous_uids);
let removed = previous_uids.difference(¤t_uids);
tracing::debug!(
- id = current.id,
+ id,
added = ?added,
"New orders in auction"
);
tracing::debug!(
- id = current.id,
+ id,
removed = ?removed,
"Orders no longer in auction"
);
- tracing::debug!(auction_id = current.id, ?start_block);
+ tracing::debug!(auction_id = id, ?start_block);
+ }
+
+ pub fn log_raw_auction_delta(
+ id: domain::auction::Id,
+ previous: Option<&domain::RawAuctionData>,
+ current: &domain::RawAuctionData,
+ start_block: &BlockInfo,
+ ) {
+ log_order_delta(
+ id,
+ previous
+ .into_iter()
+ .flat_map(|auction| auction.orders.iter().map(|order| order.uid)),
+ current.orders.iter().map(|order| order.uid),
+ start_block,
+ );
+ }
+
+ pub fn log_auction_delta(
+ previous: &Option<domain::Auction>,
+ current: &domain::Auction,
+ start_block: &BlockInfo,
+ ) {
+ log_order_delta(
+ current.id,
+ previous
+ .iter()
+ .flat_map(|auction| auction.orders.iter().map(|order| order.uid)),
+ current.orders.iter().map(|order| order.uid),
+ start_block,
+ );
}
pub fn bids(bids: &[domain::competition::Bid<Unscored>]) {
|
|
Hey @metalurgical , thanks for the suggestion, really appreciate it. Carrying the |
|
Hey @jmg-duarte, here are the benchmark results for the Branch: Benchmark ResultsRan Conversion: old vs new
The single-order regression is expected: the old impl moves heap fields for free (owned), while the new impl must clone them. The gain at 10+ orders comes from eliminating the pre-call clone that callers previously needed to retain the original. Arc::clone vs dto.clone (100 orders)
~2900x difference. If the hot path is convert-once / share-many, wrapping in Raw output |
3 participants
Description
Fixes a performance issue where
RawAuctionDatawas fully cloned on everycurrent_auction()call in the run loop. Wrapping it inArcmakes the clone operation a reference counter increment instead of a deep copy of all orders, prices, and metadata.Changes
RawAuctionDatainArcwithin the solvable orders cachecurrent_auction()to returnArc<RawAuctionData>and use cheapArc::clone()&Arc<RawAuctionData>and use.as_ref().clone()when cloning is necessarydomain::Auctionor DTO (once per auction vs. every cache access)