Lock-free latest_l2_height in gas price service
#2546
Conversation
rafal-ch
requested review from
xgreenx,
Dentosal,
MitchTurner and
rymnc
as code owners
rafal-ch
added
no changelog
| .latest_l2_height | ||
| .lock() | ||
| .map_err(|err| anyhow::anyhow!("lock error: {:?}", err))?; | ||
| let latest_l2_height = self.latest_l2_height.load(Ordering::SeqCst); |
There was a problem hiding this comment.
| let latest_l2_height = self.latest_l2_height.load(Ordering::SeqCst); | |
| let latest_l2_height = self.latest_l2_height.load(Ordering::Acquire); |
SeqCst is not needed unless you have at least two atomic variables. The purpose of SeqCst is to make sure that different threads do not see updates to different variables in different order. See https://stackoverflow.com/questions/14861822/acquire-release-versus-sequentially-consistent-memory-order for an example.
(Just mentioning this, overall I think we can keep SeqCst)
There was a problem hiding this comment.
Updated in 5eeba9e
| @@ -103,7 +107,7 @@ where | |||
| .filter_costs_that_have_values_greater_than_l2_block_height(da_block_costs)?; | |||
| tracing::debug!( | |||
| "the latest l2 height is: {:?}", | |||
| *self.latest_l2_height.lock().unwrap() | |||
| self.latest_l2_height.load(Ordering::SeqCst) | |||
There was a problem hiding this comment.
| self.latest_l2_height.load(Ordering::SeqCst) | |
| self.latest_l2_height.load(Ordering::Acquire) |
There was a problem hiding this comment.
Updated in 5eeba9e
| .lock() | ||
| .map_err(|err| anyhow!("Error locking latest L2 block: {:?}", err))?; | ||
| *latest_l2_block = BlockHeight::from(height); | ||
| self.latest_l2_block.store(height, Ordering::SeqCst); |
There was a problem hiding this comment.
| self.latest_l2_block.store(height, Ordering::SeqCst); | |
| self.latest_l2_block.store(height, Ordering::Release); |
(see the corresponding comment on the load operation)
There was a problem hiding this comment.
Updated in 5eeba9e
|
small comments because I am picky about memory ordering :) , but overall LGTM |
Description
This PR removes the
Mutexaround thelatest_l2_heightin the gas price service.Before requesting review