perf: minimize clones when saving blocks

[hai-rise]

Read another page of samply while waiting for a meeting today 😅.

There seem to be a lot of these (redundant allocations) in mempool, persistence, and root calculation hot paths. One PR at a time 🙏.

[mattsse]

very nice, appreciate this so much, ty

[shekhirin]

Are we sure that Arc::unwrap_or_clone will unwrap in these cases? From what I can see, persistence codepath gets the block from the in-memory state, which in turn isn't pruned at the same time as the persistence is advanced, and is pruned later.

[shekhirin]

That makes sense to me if Arc::unwrap_or_clone actually unwraps, deferring to engine people here.

[mattsse]

smol nit, pending sanity review @klkvr

[mattsse]

please no more newline separations, this is driving me bananas 🙃

Suggested change

[hai-rise]

Fixed 🥲.

[mattsse]

imo this makes sense

[mattsse]

unclear what this todo means, what is todo?

[hai-rise]

I meant write_hashed_state and write_trie_updates should take the full data instead of a reference just to clone it within. That only makes sense if we want to persist the data afterwards, then we only need to clone the values instead of the whole structure. However, in this case, we don't need to persist the input data so passing the whole thing in would avoid any cloning. Should I elaborate or just remove the comment for now?

[hai-rise]

Removed the comment. Will just do it in upcoming PRs that give better context anyway.

[hai-rise]

Are we sure that Arc::unwrap_or_clone will unwrap in these cases? From what I can see, persistence codepath gets the block from the in-memory state, which in turn isn't pruned at the same time as the persistence is advanced, and is pruned later.

Yeah, that's why I chose "minimize" over "remove" like in previous PRs. I think the rule of thumb is to always use Arc::unwrap_or_clone to not need to worry about a more global scope or how each function caller manages input references. The extra reference counting check is fairly cheap.

That said, looking at persist_block_after_db_tx_creation -> remove_persisted_blocks was very helpful! If we're sure successful persistence is the hot path then we can remove (with ownership instead of cloning a get) blocks from memory to persist to disk, and re-insert into memory on persistence failures. Unless we still need the blocks for something before they're persisted.

Edited: We don't need to re-insert as persistence failures are critical and shut down the node?