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

Passive elements and data are now PrimaryMaps #2183

Merged
merged 5 commits into from
Mar 12, 2021
Merged

Conversation

syrusakbary
Copy link
Member

Description

This PR moves some fields in the ModuleInfo to stop using HashMap and start using PrimaryMap as the contents are not sparse but fully dense.
The PR is incentivized by #2180 as it's partially needed to stop using HashMap when possible. It doesn't completely solve it, as the function_names field is still a HashMap, but it should be trivial to solve that in a PR that improves the deserialization time.

Review

  • Add a short description of the the change to the CHANGELOG.md file

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

First of all thanks for the description of what this PR is doing / what it's for.

I'm not sure if I believe that the data here is not sparse. It doesn't start sparse but with elem_drop it looks like it could be become sparse.

Given the diff I've just seen, it looks like the change is correct, but I'm a bit concerned about the change in semantics of this type. There could easily be code not included in this diff which doesn't do the is_empty check and is broken by this change. I think using Option instead of a sentinel value will be less error prone and more explicit. There's no runtime cost to using Option<Box<T>> instead of Box<T>, but the users will now have to deal with a potentially None value, which they were theoretically doing before.

lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Looks good! the get logic can be cleaned up a bit but otherwise 👍

lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Outdated Show resolved Hide resolved
lib/vm/src/instance/mod.rs Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors r+

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