Use Arc in HotHDiffBufferCache to avoid holding cache lock

[michaelsproul]

Issue Addressed

Partly addresses:

• Fork choice write lock held during database write #8147

Proposed Changes

Use an Arc to avoid holding the state_cache lock while HDiffBuffer::from_state runs. This conversion function can take up to 1s, so it's good if we can release the lock while it runs. This can unblock other threads waiting on the lock.

[michaelsproul]

Could consider deleting the once cell in this case, as we've created an entry and then left it empty. Alternatively we could return it for the caller to fill in once they load the state from disk.

[dapplion]

I think it's problematic to populate the LRU cache with empty cells, it can cause it to evict valuable entries for no reason. I have this commit where we only insert if necessary without much more complexity b6bd9b3

[michaelsproul]

Even better, I think we can just remove the OnceCell. I did this in this commit: a3d5354.

The reason being, the buffer cache is only meant to contain states prior to finalization, so we should not be priming it after loading a full state (which would be post finalization). In future we may alter this paradigm, but for now, just an Arc is enough.

[michaelsproul]

lighthouse/beacon_node/store/src/state_cache.rs

Lines 58 to 66 in a3d5354

	/// Cache of hdiff buffers for hot states.
	///
	/// This cache only keeps buffers prior to the finalized state, which are required by the
	/// hierarchical state diff scheme to construct newer unfinalized states.
	///
	/// The cache always retains the hdiff buffer for the most recent snapshot so that even if the
	/// cache capacity is 1, this snapshot never needs to be loaded from disk.
	#[derive(Debug)]
	pub struct HotHDiffBufferCache {

[dapplion]

Is it necessary to drop timer explicitly here?

[dapplion]

Opinionated, but it's easier to read to me and less lines using if else. I have the diff in this commit 9209c57

[michaelsproul]

I guess technically not. The timer is meant to measure just the clone, which is why it's written like this. With your change, we'll also measure the function post-amble where all of the local variables get dropped. However this should be completely negligible in terms of time, so I think it's fine

[dapplion]

There's a race condition here

• thread 1: get or insert: insert empty one
• thread 1: clone new once cell
• thread 1: start computing buffer
• thread 2: put buffer, replacing previous created once cell and wasting its compute
• thread 1: completes computing the buffer and the once cell is dropped

I don't feel it's a serious issue but noting it here

[michaelsproul]

Gone with removal of OnceCell, I think?

[dapplion]

I think it's problematic to populate the LRU cache with empty cells, it can cause it to evict valuable entries for no reason. I have this commit where we only insert if necessary without much more complexity b6bd9b3