chore: remove usage of std::mem::forget
#164
Conversation
There was a problem hiding this comment.
We forget these pointers because their destructor will be called by the C++. We don't want to free memory that has already been freed. It is not that ownership of these values are shared amongst Rust, but rather than ownership is transferred to the C++. I would rather move to something like ManuallyDrop vs. getting rid of the forget as it can lead to some funky bugs.
I see our aztec_backend tests are passing on this, but I would like this to be integration tested with Noir before these are removed.
@kevaundray does something come to mind about what may be good to test removing these forget calls?
|
If we no longer use a singleton instance, then not forgetting them could cause a memory leak -- I'm checking this now on the Keccak PR |
phated
dismissed
vezenovm’s stale review
It seems these are causing a memory leak that causes integration test crashes in Noir
There was a problem hiding this comment.
As per @kevaundray's comment and the keccak test suite passing on this CI run: https://github.com/noir-lang/noir/actions/runs/4920482191/jobs/8789322898?pr=1249
I also did a memory profile and found the pkey and vkey were not being freed.
|
Well this was good timing 😅 |
* acvm-0.12.0: fix bad rebase chore: Release 0.1.2 (#183) fix: Remove star dependencies to allow publishing (#182) chore: Release 0.1.1 (#181) fix: Add description so crate can be published (#180) chore: update readme to new name and add contract note (#177) feat!: update to acvm with non-homogeneous bb calls (#169) update to latest changes chore: Release 0.1.0 (#173) use patch syntax chore: update to use new black box solver interface feat!: update to target acvm-84b5d18d chore(ci): Update tokens (#174) chore: Add release-please and publish workflows to project (#172) feat!: Update to ACVM v0.11.0 (#151) chore: remove usage of `std::mem::forget` (#164) chore: Enforce proper conversion of memory into fixed length array (#163) chore: Add test for Keccak256 constraint (#158) chore: use `WASMValue` for wasm arguments as well as return values (#157) feat!: Add Keccak constraints (#150)
It's not clear to me on why we "forget" the CRS, PK, constraint systems and witness. I remember that previously Barretenberg would have some singleton instance of the proving key, etc. which would make some sense. We don't do this anymore and in any case, we pass in new values for each of these on every call so I don't see any reason to not to free these resources.