Skip to content

[VM] Iterative drop of vm values - #374

Open
fEst1ck wants to merge 2 commits into
movement-network:move-rebase-e33from
fEst1ck:zekun/iterative-drop
Open

[VM] Iterative drop of vm values#374
fEst1ck wants to merge 2 commits into
movement-network:move-rebase-e33from
fEst1ck:zekun/iterative-drop

Conversation

@fEst1ck

@fEst1ck fEst1ck commented Jun 4, 2026

Copy link
Copy Markdown

Description

Implements a custom drop for values to prevent overflow when dropping deeply nested values.

How Has This Been Tested?

All existing vm tests.

Type of Change

  • Bug fix

Which Components or Systems Does This Change Impact?

  • Move/Aptos Virtual Machine

musitdev
musitdev previously approved these changes Jun 4, 2026

@musitdev musitdev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine. My only remarks is that there can have some memory leak because ValueImpls can contain a ValueImpls that point to the initial ValueImpls, creating a reference cycle. So the ValueImpls are never dropped.
I don't know if it's possible in the usage but if yes the case can be avoid by using a new container enum and a struct with Weak ref like:

pub(crate) struct WeakValueImpls(Weak<RefCell<Vec<ValueImpl>>>);

enum Container {
  ...
   Parent(WeakValueImpls),
}

@fEst1ck

fEst1ck commented Jun 4, 2026

Copy link
Copy Markdown
Author

@musitdev Thanks for pointing out. Cyclic values shouldn't be constructed given the Move type system. I'll see if it's worth the extra effort to prevent against this.

@fEst1ck

fEst1ck commented Jun 4, 2026

Copy link
Copy Markdown
Author

I'll also investigate if closure dropping can be unbounded.

@seanyoung seanyoung left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is tricky, it really needs a big fat comment and a test case.

Ideally, I would like to see a different fix. I do realise this might not be simple or might cost too much time. If we allow types to be nested 40,000 deep, then surely we will encounter all sorts of problems.

I think there should be some limit on the type depth rather than working around the problems of the same types in rust (e.g. recursion in Drop). I suspect there will be other problems For example, validator CPU cycles, 40,000 malloc's and free's are very costly too.

@musitdev

musitdev commented Jun 8, 2026

Copy link
Copy Markdown

Yes, if the depth can be limited, it will be better. In fact, how big trees can be constructed in this context?

@fEst1ck
fEst1ck force-pushed the zekun/iterative-drop branch from 998fce4 to 2af6533 Compare July 15, 2026 18:24
@fEst1ck
fEst1ck requested a review from areshand as a code owner July 15, 2026 18:24
@fEst1ck
fEst1ck changed the base branch from m1 to move-rebase-e33 July 15, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants