-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reuse single shared allocation for ABI data #970
Merged
Merged
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
c596713
Very hacky attempt at making ABI allocations static
MartinquaXD 39cb153
Rename `abi -> interface` to avoid `.abi.abi`
MartinquaXD cb97d5b
Fix nightly clippy lint
MartinquaXD f0ad2d2
Fix unit test
MartinquaXD 3abea56
Fix borrow checker in commented out unit test
MartinquaXD afb668a
Remove debug logs
MartinquaXD 24610fa
Remove doc comment for hidden items (nightly clippy complains)
MartinquaXD 6a0953b
Fix clippy lints about old numbers
MartinquaXD b225965
More number cleanup
MartinquaXD c74132f
More number cleanup
MartinquaXD 098836d
fixup
MartinquaXD 5e78a27
Delete unused fields
MartinquaXD 9a3ff41
Explain manual drop with comment in code
MartinquaXD 0868463
Bump version to 0.25.6
MartinquaXD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,11 +8,12 @@ | |
//! artifact models. It also provides tools to load artifacts from different | ||
//! sources, and parse them using different formats. | ||
|
||
use crate::contract::{Documentation, Network}; | ||
use crate::contract::{Documentation, Interface, Network}; | ||
use crate::{Abi, Bytecode, Contract}; | ||
use std::collections::hash_map::Entry; | ||
use std::collections::HashMap; | ||
use std::ops::Deref; | ||
use std::sync::Arc; | ||
|
||
pub mod hardhat; | ||
pub mod truffle; | ||
|
@@ -151,7 +152,7 @@ pub struct ContractMut<'a>(&'a mut Contract); | |
impl<'a> ContractMut<'a> { | ||
/// Returns mutable reference to contract's abi. | ||
pub fn abi_mut(&mut self) -> &mut Abi { | ||
&mut self.0.abi | ||
&mut Arc::make_mut(&mut self.0.interface).abi | ||
} | ||
|
||
/// Returns mutable reference to contract's bytecode. | ||
|
@@ -188,6 +189,18 @@ impl Deref for ContractMut<'_> { | |
} | ||
} | ||
|
||
impl Drop for ContractMut<'_> { | ||
fn drop(&mut self) { | ||
// The ABI might have gotten mutated while this guard was alive. | ||
// Since we compute pre-compute and cache a few values based on the ABI | ||
// as a performance optimization we need to recompute those cached values | ||
// 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; | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
use super::*; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately those scopes are needed due to us needed to update a modified contract. (see |
||
let insert_res = artifact.insert(make_contract("C1")); | ||
|
||
assert_eq!(insert_res.inserted_contract.name, "C1"); | ||
assert!(insert_res.old_contract.is_none()); | ||
assert_eq!(insert_res.inserted_contract.name, "C1"); | ||
assert!(insert_res.old_contract.is_none()); | ||
} | ||
|
||
assert_eq!(artifact.len(), 1); | ||
assert!(artifact.contains("C1")); | ||
|
||
let insert_res = artifact.insert(make_contract("C2")); | ||
{ | ||
let insert_res = artifact.insert(make_contract("C2")); | ||
|
||
assert_eq!(insert_res.inserted_contract.name, "C2"); | ||
assert!(insert_res.old_contract.is_none()); | ||
assert_eq!(insert_res.inserted_contract.name, "C2"); | ||
assert!(insert_res.old_contract.is_none()); | ||
} | ||
|
||
assert_eq!(artifact.len(), 2); | ||
assert!(artifact.contains("C2")); | ||
|
||
let insert_res = artifact.insert(make_contract("C1")); | ||
{ | ||
let insert_res = artifact.insert(make_contract("C1")); | ||
|
||
assert_eq!(insert_res.inserted_contract.name, "C1"); | ||
assert!(insert_res.old_contract.is_some()); | ||
assert_eq!(insert_res.inserted_contract.name, "C1"); | ||
assert!(insert_res.old_contract.is_some()); | ||
} | ||
|
||
assert_eq!(artifact.len(), 2); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this panic if there is more than one
Arc
pointer to the data? Asking cuz my Rust is rusty :PThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool - so it has CoW (copy on write) semantics. Very cool, and the name is a great coincidence 😅.
I would even argue this is desired to preserve the existing mutation semantics.