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: Add Instruction::map_values_mut #6756

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Dec 10, 2024

Description

Problem*

Working towards #6751

Summary*

I noticed in a few places we use Instruction::map_values we clone the instruction beforehand to get around borrowing restrictions. This means we should mutate in place though instead of using map_values to produce another Instruction value, so I've added map_values_mut for Instruction and DataBus and searched through to find each case we could use these as well as the existing TerminatorInstruction::mutate_values which I've renamed.

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.

@jfecher jfecher requested a review from a team December 10, 2024 14:01
@jfecher jfecher changed the title map values mut chore: Add Instruction::map_values_mut Dec 10, 2024
Copy link
Contributor

github-actions bot commented Dec 10, 2024

Compilation Sample

Program Compilation Time %
sha256_regression 0m1.500s 5%
regression_4709 0m1.443s -4%
ram_blowup_regression 0m16.177s -3%
private-kernel-tail 0m1.335s -3%
private-kernel-reset 0m10.752s 4%
private-kernel-inner 0m2.906s 17%
parity-root 0m0.854s -15%
noir-contracts 2m41.256s -6%

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Peak Memory Sample

Program Peak Memory
keccak256 81.37M
workspace 121.99M
regression_4709 333.17M
ram_blowup_regression 2.53G
private-kernel-tail 232.47M
private-kernel-reset 1.24G
private-kernel-inner 338.30M
parity-root 199.36M

@TomAFrench
Copy link
Member

I'm not seeing a big benefit to the peak memory usage here although the heaptrack profiling was done on the rollup-base-private circuit which our CI doesn't currently track. I'll profile this again tomorrow to see if we should go ahead and merge.

@jfecher
Copy link
Contributor Author

jfecher commented Dec 11, 2024

@TomAFrench I'd think this change would generally benefit runtime speed more than memory use since the extra clones we did before would just get freed afterward anyway. I'm not sure why the time for private-kernel-inner seems to have noticeably increased though.

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