Skip to content

wasmparser: Implement no-hash-maps crate feature & support#1521

Merged
alexcrichton merged 73 commits intobytecodealliance:mainfrom
Robbepop:rf-no-hash-maps
May 13, 2024
Merged

wasmparser: Implement no-hash-maps crate feature & support#1521
alexcrichton merged 73 commits intobytecodealliance:mainfrom
Robbepop:rf-no-hash-maps

Conversation

@Robbepop
Copy link
Copy Markdown
Collaborator

@Robbepop Robbepop commented Apr 27, 2024 •
edited
Loading

Closes #1517 .

This PR tries to implement Set, Map IndexSet and IndexMap with as similar APIs as possible
to each other but also to Rust's HashMap, BTreeMap and indexmap::IndexMap.

TODO

  • Restructure wasmparser::map module to wasmparser::collections.
  • Opaque Set type:
    • Extended shared API between hash-set and btree-set.
    • Iterator API
  • Opaque Map type:
    • Extended shared API between hash-map and btree-map.
    • Entry API
    • Iterator API
  • Opaque IndexSet type:
    • Minimally required API
    • Iterator API
    • IndexMap<T, ()>-based custom Implementation
  • Opaque IndexMap type:
    • Minimally required API
    • Entry API
    • Iterator API
    • btree-based Implementation
      • Minimally required API
      • Entry API
      • Iterator API
      • Unit tests

Robbepop added 30 commits April 26, 2024 08:00
@Robbepop
move map.rs into its own folder
3b9be06
@Robbepop
add no-hash-maps crate feature to wasmparser
f007304 
Disabled by default.
@Robbepop
rename Hash{Map,Set} type aliases to just Map,Set
cc7910d
@Robbepop
switch to BTree{Map,Set} if no-hash-maps is enabled
db2e0b6 
This introduced a lot of compile errors in case no-hash-maps is actually enabled since many keys are not Ord.
@Robbepop
use BTree{Map,Set} type aliases with no-hash-maps
b42c67c 
Some methods are not shared between Hash{Map,Set} and BTree{Map,Set} such as reserve and the Key type has different trait bounds (Eq + Hash) vs (Eq + Ord) which can be confusing when using map and set in wasmparser during development.

Thus we actually do not want to use type aliases but proper types that wraps both Hash{Map,Set} and BTree{Map,Set} and provide a unified API.

The same is true for Index{Map,Set} but we will do this in another commit.
@Robbepop
fix intradoc links
efcfb1e
@Robbepop
cfg instead of comment out
cb38cb1 
This is just a workaround until the new Map/Set type with unified API is done.
@Robbepop
move random state definition into their own file
aa1a49e 
I want to keep the base mod.rs as clean as possible.
@Robbepop
rename map module to collections
4994129
@Robbepop
rename random_state.rs -> hash.rs and make mod pub
a149840
@Robbepop
restructure collections module
aa6ef54
@Robbepop
fix docs
af63c4c
@Robbepop
add Entry type def to map data structures
4ed8fe8
@Robbepop
update Set docs
09bfe41
@Robbepop
turn the Set type alias into an opaque type
d03b245 
Currently it only supports mostly the minimal API needed to make the wasmparser crate work.
@Robbepop
add Clone derive for Set
43d770f
@Robbepop
turn the map type into an opaque type
06fa9e7 
Currently it only mostly supports the minimal API needed to make the wasmparser crate work again.
@Robbepop
fix workspace uses of wasparser collection types
66fc03c
@Robbepop
apply rustfmt
bccf544
@Robbepop
add Iterator API for Set
e23a505
@Robbepop
add Iterator API to Map
c483b04
@Robbepop
simplify type defs and cfg guards
6e6d2fa
@Robbepop
remove unnecessary cfg guard
5373e8f
@Robbepop
simplify type aliases
02997a4
@Robbepop
add IterMut for Map
dc00238
@Robbepop
improve doc links
cf0ad84
@Robbepop
add FromIterator impls for Map and Set
1173f22 
They are used in IndexMap so for symmetry I add them here too.
@Robbepop
add Map::reserve
221e999 
symmetry: this method is used by both Set and IndexMap
@Robbepop
add IntoIterator API for Map and Set
2069437 
symmetry: used on IndexMap
@Robbepop
add Keys, Values and ValuesMut iterators for Map
9f3dfac
@Robbepop
Copy link
Copy Markdown
Collaborator Author

Robbepop commented May 8, 2024

@alexcrichton Today I have time to work on this again.

One question still open: In my Wasmi implementation I made Map and Set APIs cover the whole shared definition space to avoid hiccups during development in the future. I think this decision was pretty good despite introducing a bit more code. Shall we apply the same to wasmparser?

@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented May 8, 2024

I think it's reasonable to go ahead and add in more API surface area yeah, especially if it's easy enough to implement

@Robbepop
Copy link
Copy Markdown
Collaborator Author

Robbepop commented May 11, 2024 •
edited
Loading

@alexcrichton I have now done the following

  • implemented IndexSet in terms of IndexMap<T, ()> as suggested by you.
  • completed the shared API surface for both Set and Map types (just what I did in wasmi_collections)
  • IndexSet::replace is implemented in terms of remove and insert which is inefficient but works as intended
  • The custom IndexMap implementation is now always compiled even when it is unused for better maintainability.

Testing of the custom IndexMap implementation still needs to be done.

One question:
Shall we put #[inline] on all forwarding methods of Map, Set, IndexMap and IndexSet? I did the same for wasmi_collections but wanted to ask you first. I think for wasmparser in particular it would be beneficial since it seems that the intention is to re-export the collections module for use in other crates.

@alexcrichton
Copy link
Copy Markdown
Member

alexcrichton commented May 11, 2024

Technically the methods don't need #[inline] to be candidates for cross-crate inlining due to the use of generics, but for such small methods like these it also wont' hurt too much so if you'd like I think it's reasonable to add.

@Robbepop
Copy link
Copy Markdown
Collaborator Author

Robbepop commented May 12, 2024 •
edited
Loading

@alexcrichton I just added tests for the custom IndexMap implementation so this PR is ready for merge from my side.

I will do the #[inline] in a separate PR and make sure it actually improves performance.

alexcrichton
alexcrichton approved these changes May 13, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks again for your work on this!

@alexcrichton alexcrichton added this pull request to the merge queue May 13, 2024
Merged via the queue into bytecodealliance:main with commit fadf4e9 May 13, 2024
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request May 13, 2024
@alexcrichton
Add no-hash-maps tests to gated CI checks
5f36515 
Forgot to flag this in bytecodealliance#1521 so adding in a follow-up.
@alexcrichton alexcrichton mentioned this pull request May 13, 2024
Merged
@Robbepop
Copy link
Copy Markdown
Collaborator Author

Robbepop commented May 13, 2024 •
edited
Loading

Looks great to me, thanks again for your work on this!

Thank you for making wasmparser compatible with Wasmi. I am very happy to contribute this. :)

github-merge-queue Bot pushed a commit that referenced this pull request May 14, 2024
@alexcrichton
Add no-hash-maps tests to gated CI checks (#1556)
4679411 
Forgot to flag this in #1521 so adding in a follow-up.
@Robbepop Robbepop mentioned this pull request May 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

wasmparser: Add no-hash-maps crate feature & support

2 participants

@Robbepop @alexcrichton