-
Notifications
You must be signed in to change notification settings - Fork 138
Remove sandbox evolving and devolving and replace it with snapshotting API. #697
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
Conversation
cbf3729
to
1106a4e
Compare
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.
Great work, @jprendes!
I like when I see more code removed than it was added 😃
The PR description is a bit confusing, mentioning the removal of evolve/devolve APIs, but there is still evolve
ing involved 😄
I left some comments here and there where I didn't understand something.
NOTE: Some of the comments may be outdated as I reviewed each commit.
Haha, yes, indeed. At lest the traits are not there anymore. |
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.
LGTM
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
…y_name Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
Signed-off-by: Jorge Prendes <[email protected]>
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.
Looks good to me apart from some very minor comments.
I agree with @jsturtevant that there is some worry about whether code will continue compiling but do something different, which would be extremely unfortunate. However, I would like to get this merged so that it can unblock other work that would ideally be (re)based on top of this, so I personally at least am happy to leave working out the exact details of what should be done there to a follow-up issue that we consider a blocker for release.
This PR introduces a first version of the snapshot API.
The current implementation doesn't play very nicely with memory mapping. This is because restoring a snapshot will not restore the mapped memory. As this PR already introduces many other changes, I would prefer to keep fixing the interaction with memory mapping to a different PR.
I recommend reviewing it commit by commit. Each commit should be fairly self contained.
This PR introduces a public snapshot API.
As a result, a sandbox is not automatically snapshotted and restored anymore. It has to be triggered by the user instead.
Before we were snapshotting a sandbox when evolving it, and restoring it after each guest call.
As a result, we do not need the evolve / devolve mechanism anymore.
Evolving allowed us to persist changes in a sandbox, now this happens with all guest calls.
Devolving was not being used anywhere AFAIK.
As the evolving/devolving API is removed, we don't need the transition metadata anymore (which was used by evolving), and neither the noop transition.
We can also remove the Sandbox trait that by this point has lost all but one of its methods, which can be easily inlined in the only place where it was used.
We don't need the call context anymore either.