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.

Sign up for GitHub

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

Jump to bottom

chore: remove dependency on generational-arena #4207

Merged
merged 29 commits into from
Feb 16, 2024
Merged

chore: remove dependency on generational-arena #4207

merged 29 commits into from
Feb 16, 2024

Conversation

michaeljklein
Copy link
Contributor

Description

Problem*

Resolves #15 (comment)

Summary*

Replaces generational-arena's Arena class with a thin wrapper around Vec

Additional Context

The thin wrapper is helpful for:

  1. insert returning the index
  2. iter iterating over the (index, item) pairs

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

jfecher
jfecher reviewed Jan 30, 2024
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

LGTM. Unsure what the errors in the stdlib are coming from seeing as this PR doesn't actually change any of the method interface.

In particular std::wrapping_add seems to have somehow been given the type [Field; 16]... odd

compiler/utils/arena/src/lib.rs Outdated Show resolved Hide resolved
@kevaundray kevaundray changed the title wip: remove dependency on generational-arena chore: remove dependency on generational-arena Feb 1, 2024
@kevaundray kevaundray marked this pull request as draft February 1, 2024 12:26
@kevaundray
Copy link
Contributor

Removed wip from pr scope and put PR as a draft

@jfecher
Copy link
Contributor

jfecher commented Feb 12, 2024

This PR is working locally now for me

@jfecher
Copy link
Contributor

jfecher commented Feb 13, 2024

@michaeljklein is this ready for review now? Or are we waiting until the generational index is removed from Index?

@michaeljklein
Copy link
Contributor Author

@jfecher it's ready for review

@michaeljklein michaeljklein marked this pull request as ready for review February 13, 2024 22:46
jfecher
jfecher reviewed Feb 14, 2024
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

A few comments. - There is also the PartialEq comment from above

compiler/noirc_frontend/src/hir/def_map/item_scope.rs Outdated Show resolved Hide resolved
compiler/noirc_frontend/src/hir/def_map/module_data.rs Outdated Show resolved Hide resolved
compiler/noirc_frontend/src/hir/type_check/mod.rs Outdated Show resolved Hide resolved
@jfecher
Copy link
Contributor

jfecher commented Feb 15, 2024

I've found the origin of the errors in this PR. In id_to_type we're overwriting ExprId's types with DefinitionId's types and vice-versa because of the impl From<DefinitionId> for Index. In converting a definitionId to an index, the indices used may overlap since they originate from different maps. So when inserting them both into id_to_type we overwrite types and everything breaks.

jfecher added a commit that referenced this pull request Feb 15, 2024
@jfecher
Initial work on fixing #4207
63f627b
@jfecher jfecher mentioned this pull request Feb 16, 2024
Merged
5 tasks
jfecher and others added 3 commits February 16, 2024 13:06
@jfecher
fix: DefinitionIds should not be stored in id_to_type (#4397)
e49fde4 
# Description

## Problem\*

Resolves the underlying issue with #4207 where definition/expr types are
being overwritten by other exprs/definitions because DefinitionIds are
converted into Indexes which may clash with those from expressions.

## Summary\*

Separates out `id_to_type` into another map for only definitions:
`definition_to_type` to resolve the clash. I've also removed the `impl
From<DefinitionId> for Index` to prevent this issue in the future.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@michaeljklein
cleanup debugging, remove generational arena note from deny.toml
7885574
@michaeljklein
Merge branch 'master' into michaeljklein/vec-arena
881373d
jfecher
jfecher approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Thanks!

@jfecher jfecher added this pull request to the merge queue Feb 16, 2024
Merged via the queue into master with commit 46f2204 Feb 16, 2024
48 checks passed
@jfecher jfecher deleted the michaeljklein/vec-arena branch February 16, 2024 19:47
TomAFrench added a commit that referenced this pull request Feb 26, 2024
@TomAFrench
Merge branch 'master' into aztec-packages
fac5770 
* master: (39 commits)
  chore: remove unwanted prints (#4419)
  fix: remove print from monomorphization pass (#4417)
  chore(ssa): Remove mem2reg run before flattening (#4415)
  feat: Add HashMap to the stdlib (#4242)
  fix!: Ban Fields in for loop indices and bitwise ops (#4376)
  chore: Add #[recursive] Explainer to Documentation (#4399)
  feat(ci): Use wasm-opt when compiling wasm packages (#4334)
  fix: add handling to `noir_wasm` for projects without dependencies (#4344)
  chore: rename parameter 'filter' to 'level' in 'init_log_level' (#4403)
  chore!: bump msrv to 1.73.0 (#4406)
  chore: fix docker test workflows (#4308)
  chore: Update Vec docs (#4400)
  feat: update error message when trying to load workspace as dependency (#4393)
  chore: bump webpack dependencies (#4346)
  fix: correct invalid brillig codegen for `EmbeddedCurvePoint.add` (#4382)
  chore: remove dependency on generational-arena (#4207)
  fix(docs): update install versions (#4396)
  fix: Enforce matching types of binary ops in SSA (#4391)
  fix(docs): Update noirjs_app for 0.23 (#4378)
  chore(ci): add alerts for failed publishes (#4388)
  ...
TomAFrench added a commit that referenced this pull request Feb 26, 2024
@TomAFrench
Merge branch 'master' into kw/change-to-lowercase-field
630cbc1 
* master: (440 commits)
  chore: remove duplicate `parse_all` function in wasm compiler (#4411)
  chore(ci): prevent msrv checks from blocking PRs (#4414)
  feat: expose separate functions to compile programs vs contracts in `noir_wasm` (#4413)
  chore: do not panic when dividing by zero (#4424)
  chore: remove unwanted prints (#4419)
  fix: remove print from monomorphization pass (#4417)
  chore(ssa): Remove mem2reg run before flattening (#4415)
  feat: Add HashMap to the stdlib (#4242)
  fix!: Ban Fields in for loop indices and bitwise ops (#4376)
  chore: Add #[recursive] Explainer to Documentation (#4399)
  feat(ci): Use wasm-opt when compiling wasm packages (#4334)
  fix: add handling to `noir_wasm` for projects without dependencies (#4344)
  chore: rename parameter 'filter' to 'level' in 'init_log_level' (#4403)
  chore!: bump msrv to 1.73.0 (#4406)
  chore: fix docker test workflows (#4308)
  chore: Update Vec docs (#4400)
  feat: update error message when trying to load workspace as dependency (#4393)
  chore: bump webpack dependencies (#4346)
  fix: correct invalid brillig codegen for `EmbeddedCurvePoint.add` (#4382)
  chore: remove dependency on generational-arena (#4207)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevaundray kevaundray kevaundray left review comments

@TomAFrench TomAFrench TomAFrench left review comments

@jfecher jfecher jfecher 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.

Remove generational arenas
4 participants
@michaeljklein @kevaundray @jfecher @TomAFrench