From 88b37a10df251f457ae802a06439d27fdb009aa0 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 29 Feb 2024 12:51:07 +1100 Subject: [PATCH] Optimise no-op PATCH ops in VC HTTP API (#5064) * Optimise no-op changes in VC API * Handle another no-op case * Merge remote-tracking branch 'origin/unstable' into opt-vc-patch-api --- validator_client/src/http_api/mod.rs | 80 +++++++++++++++++++++------- 1 file changed, 61 insertions(+), 19 deletions(-) diff --git a/validator_client/src/http_api/mod.rs b/validator_client/src/http_api/mod.rs index dcf66d2fb..a4480195e 100644 --- a/validator_client/src/http_api/mod.rs +++ b/validator_client/src/http_api/mod.rs @@ -685,7 +685,16 @@ pub fn serve( let maybe_graffiti = body.graffiti.clone().map(Into::into); let initialized_validators_rw_lock = validator_store.initialized_validators(); - let mut initialized_validators = initialized_validators_rw_lock.write(); + let initialized_validators = initialized_validators_rw_lock.upgradable_read(); + + // Do not make any changes if all fields are identical or unchanged. + fn equal_or_none( + current_value: Option, + new_value: Option, + ) -> bool { + new_value.is_none() || current_value == new_value + } + match ( initialized_validators.is_enabled(&validator_pubkey), initialized_validators.validator(&validator_pubkey.compress()), @@ -694,32 +703,65 @@ pub fn serve( "no validator for {:?}", validator_pubkey ))), + // If all specified parameters match their existing settings, then this + // change is a no-op. (Some(is_enabled), Some(initialized_validator)) - if Some(is_enabled) == body.enabled - && initialized_validator.get_gas_limit() == body.gas_limit - && initialized_validator.get_builder_boost_factor() - == body.builder_boost_factor - && initialized_validator.get_builder_proposals() - == body.builder_proposals - && initialized_validator.get_prefer_builder_proposals() - == body.prefer_builder_proposals - && initialized_validator.get_graffiti() == maybe_graffiti => + if equal_or_none(Some(is_enabled), body.enabled) + && equal_or_none( + initialized_validator.get_gas_limit(), + body.gas_limit, + ) + && equal_or_none( + initialized_validator.get_builder_boost_factor(), + body.builder_boost_factor, + ) + && equal_or_none( + initialized_validator.get_builder_proposals(), + body.builder_proposals, + ) + && equal_or_none( + initialized_validator.get_prefer_builder_proposals(), + body.prefer_builder_proposals, + ) + && equal_or_none( + initialized_validator.get_graffiti(), + maybe_graffiti, + ) => + { + Ok(()) + } + // Disabling an already disabled validator *with no other changes* is a + // no-op. + (Some(false), None) + if body.enabled.map_or(true, |enabled| !enabled) + && body.gas_limit.is_none() + && body.builder_boost_factor.is_none() + && body.builder_proposals.is_none() + && body.prefer_builder_proposals.is_none() + && maybe_graffiti.is_none() => { Ok(()) } (Some(_), _) => { + // Upgrade read lock only in the case where a write is actually + // required. + let mut initialized_validators_write = + parking_lot::RwLockUpgradableReadGuard::upgrade( + initialized_validators, + ); if let Some(handle) = task_executor.handle() { handle .block_on( - initialized_validators.set_validator_definition_fields( - &validator_pubkey, - body.enabled, - body.gas_limit, - body.builder_proposals, - body.builder_boost_factor, - body.prefer_builder_proposals, - body.graffiti, - ), + initialized_validators_write + .set_validator_definition_fields( + &validator_pubkey, + body.enabled, + body.gas_limit, + body.builder_proposals, + body.builder_boost_factor, + body.prefer_builder_proposals, + body.graffiti, + ), ) .map_err(|e| { warp_utils::reject::custom_server_error(format!(