db-api: trait Compress and Encode use reference self

[nkysg]

ref #11185.

Trait Encode fn encodes use self. It's a bit weird.

I see many crate use &self, like

This is alloyed rlp Encode trait.
https://github.com/alloy-rs/rlp/blob/9aef28e6da7c2769460dd6bf22eeacd53adf0ffc/crates/rlp/src/encode.rs#L15

/// A type that can be encoded via RLP.
pub trait Encodable {
    /// Encodes the type into the `out` buffer.
    fn encode(&self, out: &mut dyn BufMut);

    /// Returns the length of the encoding of this type in bytes.
    ///
    /// The default implementation computes this by encoding the type.
    /// When possible, we recommender implementers override this with a
    /// specialized implementation.
    #[inline]
    fn length(&self) -> usize {
        let mut out = Vec::new();
        self.encode(&mut out);
        out.len()
    }
}

And then maybe we can remove some clone. I think some db fn like put also can use reference

[joshieDo]

headers, tx_lookup and hashing_account stages use this heavily, tbh don't know the impact of cloning for each row

wdyt @DaniPopes

[emhane]

better to return &Self::Compressed and letting caller context clone, if I understood the issue right, this is happening anyway on main?

always better to call clone in context using return value, so the dev is conscious of this perf downgrade wherever it's used.

[joshieDo]

same

[DaniPopes]

This can only work if we propagate the reference all the way to all call sites (taking in &Table::Value by reference everywhere), and if type Encoded is changed to have a lifetime so we can return a reference to self in simple cases like &Vec<u8> -> &[u8]; even then, if we always have an owned key at all call sites this is not going to change a whole lot

[DaniPopes]

@nkysg Hey actually I didn't consider another usecase, which is taking by reference in the DbTxMut methods and alike; this allows re-using buffers like so:

diff --git a/crates/storage/provider/src/providers/database/provider.rs b/crates/storage/provider/src/providers/database/provider.rs
index 7159720bf..9efba8805 100644
--- a/crates/storage/provider/src/providers/database/provider.rs
+++ b/crates/storage/provider/src/providers/database/provider.rs
@@ -1340,6 +1340,7 @@ impl<TX: DbTxMut + DbTx, Spec: Send + Sync> DatabaseProvider<TX, Spec> {
         P: Copy,
         T: Table<Value = BlockNumberList>,
     {
+        let mut number_list = BlockNumberList::empty();
         for (partial_key, indices) in index_updates {
             let mut last_shard =
                 self.take_shard::<T>(sharded_key_factory(partial_key, u64::MAX))?;
@@ -1354,9 +1355,11 @@ impl<TX: DbTxMut + DbTx, Spec: Send + Sync> DatabaseProvider<TX, Spec> {
                     // Insert last list with `u64::MAX`.
                     u64::MAX
                 };
+                number_list.clear();
+                number_list.append(list.iter().copied());
                 self.tx.put::<T>(
                     sharded_key_factory(partial_key, highest_block_number),
-                    BlockNumberList::new_pre_sorted(list.iter().copied()),
+                    &number_list,
                 )?;
             }
         }

I think we should keep going with this, do you mind picking this up again? If not I'll open an issue for it

[nkysg]

@DaniPopes Got it. Thanks sir. pleasure to give me some suggestions to improve this. and I have opened this issue #11185 again.