Skip to content
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

Memory leak in forge vm.snapshot and vm.revertTo #6411

Closed
2 tasks done
maa105 opened this issue Nov 23, 2023 · 9 comments
Closed
2 tasks done

Memory leak in forge vm.snapshot and vm.revertTo #6411

maa105 opened this issue Nov 23, 2023 · 9 comments
Assignees
Labels
good first issue Good for newcomers T-bug Type: bug

Comments

@maa105
Copy link

maa105 commented Nov 23, 2023

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (7b45265 2023-11-21T00:22:00.899582254Z)

What command(s) is the bug in?

forge test

Operating System

Linux

Describe the bug

We have a huge test case where we have 1000s of cases and each with lots of vm.snapshot and vm.revertTo calls. We noticed the memory consumption increases when time goes so we did a small test to confirm it

contract SnapshotTest is Test {
    function testSnapshots() public {
        uint preState;
        for(uint c = 0; c < 1000; c++) {
            for(uint cc = 0; cc < 10000; cc++) {
                preState = vm.snapshot();
                vm.revertTo(preState);
            }
            vm.sleep(2500);
        }
    }
}

What this code does is create a snapshot and revert directly in a loop then sleep for 2.5 seconds. If you run this you'll see the memory usage increases every 2-3 seconds consistently.

While digging around, we noticed that in your documentation here you mentioned that reverting a snapshot deletes the snapshot and all subsequent ones. However, we also noticed that this PR seems to do the opposite. We suspect that the memory leak is related to this PR.

On the short term is there a way to delete/clean the snapshots?

@maa105 maa105 added the T-bug Type: bug label Nov 23, 2023
@gakonst gakonst added this to Foundry Nov 23, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Nov 23, 2023
@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

thanks for this, checking shortly.

On the short term is there a way to delete/clean the snapshots?

there's not but we can certainly add a new cheatcode for that. do you want to try this?

@maa105
Copy link
Author

maa105 commented Nov 23, 2023

there's not but we can certainly add a new cheatcode for that. do you want to try this?

Thanks you for the prompt response. Yes please that would be very helpful

@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

oh I think I remember why we did this, it was just more convenient than the old behaviour but we def need a cheatcode to clean snapshots then

cc @Evalir

@maa105
cheatcodes are defined like this

/// Revert the state of the EVM to a previous snapshot
/// Takes the snapshot ID to revert to.
/// This deletes the snapshot and all snapshots taken after the given snapshot ID.
#[cheatcode(group = Evm, safety = Unsafe)]
function revertTo(uint256 snapshotId) external returns (bool success);

so I think we want a new cheatcode clearSnapshots()

and an implementation like revertTo that clears them:

impl Cheatcode for revertToCall {
fn apply_full<DB: DatabaseExt>(&self, ccx: &mut CheatsCtxt<DB>) -> Result {
let Self { snapshotId } = self;
let result = if let Some(journaled_state) =
ccx.data.db.revert(*snapshotId, &ccx.data.journaled_state, ccx.data.env)
{
// we reset the evm's journaled_state to the state of the snapshot previous state
ccx.data.journaled_state = journaled_state;
true
} else {
false
};
Ok(result.abi_encode())
}
}

perhaps even an additional cheatcode RevertToAndRemove or something that reverts and deletes

do you feel comfortable with giving this a try?

doesn't have to be perfect, a draft with cheatcodes would be sufficient

/// This deletes the snapshot and all snapshots taken after the given snapshot ID.

docs also need to be updated

@maa105
Copy link
Author

maa105 commented Nov 23, 2023

Thanks. Ill give it a shot tomorrow

@maa105
Copy link
Author

maa105 commented Nov 24, 2023

Reverting the commit of #5487 improved the memory leak meaning the increase in memory over time decreased however there is still memory leak :/

@Evalir
Copy link
Member

Evalir commented Nov 24, 2023

hey @maa105 — yep, reverting #5487 should alleviate the problem, but this is not something we want to do—we have users who depend on the new behavior (and we should update the docs) as it's much more convenient to not have snapshots deleted unless specifically requested. The solution to this is what @mattsse proposed

@maa105
Copy link
Author

maa105 commented Nov 24, 2023

The PR is indeed very useful however it worsens the problem of memory leak. Is there a way to reset all memory usages inside the test. I.e. mid way inside a test id like to nuke all memory and return to the state when the test started.

@mattsse
Copy link
Member

mattsse commented Nov 24, 2023

#6411 (comment)

@mattsse mattsse added the good first issue Good for newcomers label Nov 27, 2023
karlb added a commit to celo-org/optimism that referenced this issue Nov 28, 2023
I measured the required mem usage and it is 8545MB on CircleCI at the
moment. I'm not sure why upstream needs so much less.

The high memory usage might be caused by foundry-rs/foundry#6411
karlb added a commit to celo-org/optimism that referenced this issue Nov 28, 2023
I measured the required mem usage and it is 8545MB on CircleCI at the
moment. I'm not sure why upstream needs so much less.

The high memory usage might be caused by foundry-rs/foundry#6411
karlb added a commit to celo-org/optimism that referenced this issue Nov 29, 2023
I measured the required mem usage and it is 8545MB on CircleCI at the
moment. I'm not sure why upstream needs so much less.

The high memory usage might be caused by foundry-rs/foundry#6411
@Evalir
Copy link
Member

Evalir commented Dec 14, 2023

Closed by #6548

@Evalir Evalir closed this as completed Dec 14, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers T-bug Type: bug
Projects
Archived in project
Development

No branches or pull requests

3 participants