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

chore: Remove Type from Value, and remove ctrl_typevars #6771

Closed
wants to merge 12 commits into from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 11, 2024

Description

Problem*

Summary*

Another step towards removing ValueId.

Removes Type from the Value enum to make Values copyable by moving types to be stored in Instructions when they weren't before. This means every instruction knows its result type(s) and ctrl_typevars are never needed

Unlike #6769, this PR is a draft because I need to check performance - without the larger change to use a Value enum instead of ValueIds, this PR may result in worse performance alone.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] 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.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

Changes to Brillig bytecode sizes

Generated at commit: e568505f9d1a1cd54bd893ccff7a7e35e7d4c474, compared to commit: f065c6682e2c896a346716cf88ac285f1d4bf846

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
to_le_bytes +10 ❌ +8.20%

Full diff report 👇
Program Brillig opcodes (+/-) %
to_le_bytes 132 (+10) +8.20%

Copy link
Contributor

github-actions bot commented Dec 11, 2024

Peak Memory Sample

Program Peak Memory %
keccak256 78.93M 0%
workspace 121.94M -1%
regression_4709 297.21M 3%
ram_blowup_regression 1.62G 0%
private-kernel-tail 209.59M 0%
private-kernel-reset 848.52M 0%
private-kernel-inner 304.36M 0%
parity-root 175.01M 0%

Copy link
Contributor

github-actions bot commented Dec 11, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.440s -2%
regression_4709 0m0.698s -12%
ram_blowup_regression 0m16.057s -2%
rollup-base-public 5m46.108s 17%
rollup-base-private 3m17.497s 1%
private-kernel-tail 0m1.163s -18%
private-kernel-reset 0m8.771s -2%
private-kernel-inner 0m2.433s -4%
parity-root 0m0.937s -5%
noir-contracts 2m34.915s -11%

Base automatically changed from jf/numeric-type to master December 11, 2024 17:47
@jfecher
Copy link
Contributor Author

jfecher commented Dec 14, 2024

Surprised this seems to improve compilation time for some programs. I'm just going to close this PR though and include this as part of the larger change to remove ValueIds.

@jfecher jfecher closed this Dec 14, 2024
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