Reuse single shared allocation for ABI data #970
Conversation
| @@ -204,26 +217,32 @@ mod test { | |||
|
|
|||
| assert_eq!(artifact.len(), 0); | |||
|
|
|||
| let insert_res = artifact.insert(make_contract("C1")); | |||
| { | |||
There was a problem hiding this comment.
Unfortunately those scopes are needed due to us needed to update a modified contract. (see impl Drop for ContractMut<'_>.
MartinquaXD
changed the title
| // with the new ABI once the user is done updating the mutable contract. | ||
| let abi = self.0.interface.abi.clone(); | ||
| let interface = Interface::from(abi); | ||
| *Arc::make_mut(&mut self.0.interface) = interface; |
There was a problem hiding this comment.
Can this panic if there is more than one Arc pointer to the data? Asking cuz my Rust is rusty :P
There was a problem hiding this comment.
No, this function is actually pretty cool. If there are other Arcs this will clone the current content of the Arc and make it point to the new allocation that can now safely be modified. If it's the only Arc it will mutate the content in place. It's slightly wonky because you could clone the original Arc, update the artifact and expect that your cloned Arc would see the updates but this is not the case. I think this is fine, though since fundamentally this behavior was already present before factoring in borrowing rules.
There was a problem hiding this comment.
Oh, cool - so it has CoW (copy on write) semantics. Very cool, and the name is a great coincidence 😅.
I think this is fine, though since fundamentally this behavior was already present before factoring in borrowing rules.
I would even argue this is desired to preserve the existing mutation semantics.
# Description Bump `ethcontract-rs` version to make use of this performance improvement: cowprotocol/ethcontract-rs#970 ## How to test CI existing tests
# Description Bump `ethcontract-rs` version to make use of this performance improvement: cowprotocol/ethcontract-rs#970 ## How to test CI existing tests
Currently whenever you create a new instance of a contract the entire
Abigets cloned (which can be really huge) plus a bunch of signatures get computed (a lot of hashing) and cached. This is really costly considering that theAbiof a contract never changes so it would be an easy win to share the same allocation of those immutable pieces of data (Abiand computed hashes) across all instances of the same contract. This would make the creation of subsequent instances of the same contract mostly free.This can already be achieved with ugly workarounds discussed and implemented here but if this issue gets resolved in the library itself manual allocation optimization will not be needed and the performance improvement for any code that creates a lot of contract instances would be huge.
The actual change is overall pretty small. I created a new type
Interfacethat contains all the immutable data derived from theAbi. Then I gave it custom(De)Serializeimplementations which mostly just do the same thing it did before (defer toAbi's implementation). TheDeserializeimplementation of course also computes the important hash and stores it's values together with theAbi.Because the typesafe rust binding code generated by
ethcontract-rsalready stores the raw contract in a static allocation (which now contains the relevant immutable stateArced) we now get cheap contract instantiations.Thanks @nlordell for the idea of fixing the performance issue directly in the library. 👍
The code also has a few unrelated changes to make the
nightlyCI build pass.Breaking changes
Artifact::insertcan now run into annoying borrow checker issues (that can be worked around, though)Contract:abiis now accessible withContract::interface::abiContract::interface::abiis harder to mutate thanContract::abidue to the introduction of theArcOverall I don't know how many people actually rely on this crate and whether or not these breaking changes would be a big deal for them. But even if this should not get merged into the main release of the crate I think this patch is still useful for anybody who needs to optimize performance across the board.
Test Plan
existing tests
ran cow protocol backend using the patched library and didn't see any obvious issues