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

Wrap MastForest in Program and Library in Arc #1465

Merged
merged 9 commits into from
Aug 27, 2024
Merged

Conversation

bobbinth
Copy link
Contributor

@bobbinth bobbinth commented Aug 23, 2024

This PR wraps MastForest fields in Program and Library structs in Arc. This way, cloning these objects becomes a relatively cheap operation.

Additionally, this PR contains the following changes/refactorings:

  1. A check was added to make sure that an entrypoint of a Program is registered as a root in the MastForest. This was already happening - but now we have an explicit check for this.
  2. For every re-exported procedure, we now add an External node to the MastForest. This makes Library and Program structs a bit more consistent and also removes a somewhat un-intuitive situations where if a library had only re-exported procedures, its MastForest would be completely empty. Since now there is an easy way to track re-exported procedures in the MastForest itself, the distinction between local and re-exported procedures was removed from the Library struct (but I added a method that allows checking if a given export is a re-export).
  3. In the MastForestBuilder, we now insert Procedure structs for re-exported procedures too. This allowed removing procedure_hashes map and unify the way we treat local and re-exported procedures.

There is one thing we can improve in the future: if we re-export a procedure from the library and a procedure with the same digest is locally defined, we may end up with some unreachable nodes in the MAST forest. This is technically not a bug - but it does create bloat in the MAST forest. In general, I think if a library has a locally-defined procedure and also re-exports a procedure with the same root, the re-exported procedure should take precedence.

This PR closes #1430. As suggested by @bitwalker, I did not to add Version to the Library. We should probably either remove Version entirely or move it to miden-core. But I left this for future PRs.

@bobbinth bobbinth marked this pull request as ready for review August 25, 2024 07:16
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few nits

assembly/src/library/mod.rs Outdated Show resolved Hide resolved
stdlib/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/assembler/mod.rs Show resolved Hide resolved
@bobbinth bobbinth merged commit b192c61 into next Aug 27, 2024
9 checks passed
@bobbinth bobbinth deleted the bobbin-arc-mast branch August 27, 2024 20:11
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.

2 participants