feat: Optimise HDiffBuffer by using Arc instead of Vec

[PoulavBhowmick03]

Issue Addressed

Fixes #6579

Proposed Changes

Modified HDiffBuffer to use Vex<Arc<Validator>> instead of Vec<Validator>, and made necessary changes regarding that

[michaelsproul]

I don't see how using Arc<[Validator]> helps when we do this conversion to and from Vec<Validator> whenever we apply a diff.

Are you trying to save memory or CPU cycles?

[PoulavBhowmick03]

You're right. i was trying to save memory as the issue mentioned, but converting to Vec<> fails that purpose.

[michaelsproul]

Oh yeah I forgot about that issue, I think the structure I was imagining was Vec<Arc<Validator>>

[PoulavBhowmick03]

Got it. It has to be a collection of individual Arcs. Made a mistake on wrapping all the validators in a single Arc. Will be changing that

[michaelsproul]

If we did want to save memory, a promising approach might be to keep the List, which is a copy-on-write (persistent) data structure

However, Lists have slower O(log n) indexing than vecs, and we do random-access indexing here:

lighthouse/beacon_node/store/src/hdiff.rs

Line 553 in 6be646c

if let Some(x) = xs.get_mut(index as usize) {

We also do it here, but this use could be replaced by a zip on two iterators:

lighthouse/beacon_node/store/src/hdiff.rs

Line 461 in 6be646c

let validator_diff = if let Some(x) = xs.get(i) {

[michaelsproul]

This is maybe OK, but we could make it even better if we added an iter_arc method to List. In the case of validators, they are not packed in leaves, and are stored behind an Arc:

https://github.com/sigp/milhouse/blob/6da903ce8f783295714a68de4a2dcfc8b7c6ee01/src/leaf.rs#L17

Would you be interested in making a PR to milhouse to add this? I think the API would have to consider the case where T is packed, and return an error in this case. Something like:

pub fn iter_arc(&self) -> Result<impl Iterator<Item = &Arc<T>>, Error> {
   if T::tree_hash_type() == TreeHashType::Basic {
       // Can't return `Arc`s for packed leaves.
       return Err(Error::PackedLeavesNoArc);
   }
   // TODO
   Ok(iterator)
}

I'm happy to help advise on the implementation. Take a look at the other iterators in:

• https://github.com/sigp/milhouse/blob/main/src/iter.rs
• https://github.com/sigp/milhouse/blob/main/src/interface_iter.rs

[PoulavBhowmick03]

Sure! I will look into it and try to implement the iter_arc method to List. Leaving a comment on the issue in Milhouse

[michaelsproul]

Thanks!

[mergify]

Hi @PoulavBhowmick03, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify]

Hi @PoulavBhowmick03, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.