[move][move-vm][rewrite][cleanup 4/n] Replace AST Identifiers with IdentifierKey#21275
Merged
cgswords merged 2 commits intobella-ciaofrom Feb 25, 2025
Merged
[move][move-vm][rewrite][cleanup 4/n] Replace AST Identifiers with IdentifierKey#21275cgswords merged 2 commits intobella-ciaofrom
cgswords merged 2 commits intobella-ciaofrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
cgswords
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
tzakian
approved these changes
Feb 19, 2025
external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs
Outdated
Show resolved
Hide resolved
cgswords
force-pushed
the
cgswords/vm_pointer_exec
branch
from
3418245 to
5e0a343
Compare
cgswords
force-pushed
the
cgswords/vm_ast_ident_keys
branch
from
fd94480 to
43a3541
Compare
cgswords
requested review from
amnn,
emmazzz,
ronny-mysten,
stefan-mysten and
wlmyng
as code owners
cgswords
force-pushed
the
cgswords/vm_ast_ident_keys
branch
from
43a3541 to
bdac164
Compare
cgswords
changed the base branch from
cgswords/vm_pointer_exec
to
tzakian/vm-rewrite-adapter-15
cgswords
force-pushed
the
cgswords/vm_ast_ident_keys
branch
from
bdac164 to
5bddab4
Compare
cgswords
requested review from
a team,
joyqvq,
mystenmark and
patrickkuo
as code owners
cgswords
requested review from
anwayaa and
pchrysochoidis
and removed request for
a team
cgswords
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
cgswords
force-pushed
the
cgswords/vm_ast_ident_keys
branch
from
5bddab4 to
b400c71
Compare
cgswords
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
tzakian
changed the base branch from
tzakian/vm-rewrite-adapter-15
to
bella-ciao
tzakian
approved these changes
Feb 21, 2025
| /// modules. It exposes an intentionally spartan interface to prevent any unexpected behavior | ||
| /// (e.g., unstable iteration ordering) that Rust's standard collections run afoul of. | ||
| #[derive(Debug)] | ||
| pub struct DefinitionMap<Value>(HashMap<IntraPackageKey, Value>); |
| self.name.to_string().unwrap() | ||
| } | ||
|
|
||
| #[cfg(any(debug_assertions, feature = "tracing"))] |
Contributor
There was a problem hiding this comment.
Just a note (nothing for you to do): I'm going to be removing this cfg in another PR here soon -- runs afoul of building in release mode without tracing enabled since this function is used in the Debug implementation for bytecode.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
This replaces
Identifiers in the execution AST withIdentifierKey, because identifiers (and the strings they hold) led to memory leaks -- the string bytes were heap-allocated, so leaked when the Identifiers were dropped.This also refactors the interner to be more self-contained.
Test plan
Tests all pass, plus this command reported no leaks:
This now reports no leaks.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.