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

feat: Add basic benchmarking #2213

Merged
merged 23 commits into from
Aug 11, 2023
Merged

feat: Add basic benchmarking #2213

merged 23 commits into from
Aug 11, 2023

Conversation

Ethan-000
Copy link
Contributor

@Ethan-000 Ethan-000 commented Aug 8, 2023 •
edited
Loading

Description

Problem*

Resolves

Summary*

run cargo bench --bench iai
will result in something like this

iai_selected_tests_execution
  Instructions:              195685 (No change)
  L1 Accesses:               269771 (No change)
  L2 Accesses:                 1343 (No change)
  RAM Accesses:                 565 (No change)
  Estimated Cycles:          296261 (No change)

this is the aggregated result of all the selected tests

run cargo bench --bench criterion

for common criterion result

iai can be used for pr benchmark comparisons, i think i can do this once barretenberg isn't required to build & execute noir programs (currently it takes too long to install it in ci)

Additional Context

PR Checklist*

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

@kevaundray
Copy link
Contributor

What is the output of this job btw? For example, what happens when I submit a PR and what happens when a PR regresses performance?

@Ethan-000
Copy link
Contributor Author

What is the output of this job btw? For example, what happens when I submit a PR and what happens when a PR regresses performance?

ehh idk, probably an error if the diff is too big, lemme check 🥲

@Ethan-000 Ethan-000 assigned Ethan-000 and unassigned Ethan-000 Aug 8, 2023
@Ethan-000
Copy link
Contributor Author

Ethan-000 commented Aug 8, 2023 •
edited
Loading

I think it will output an error if the performace is 1% or more worse after the pr

@Ethan-000 Ethan-000 marked this pull request as draft August 8, 2023 21:01
@Ethan-000
Copy link
Contributor Author

iai 0.1.1 does not work with valgrind 3.21.0 see issue 34 of https://github.com/bheisler/iai

idk whats the version of valgrind on ubuntu. i guess its fine cus reth is using this setup. if valgrind updates on ubuntu may need to switch to a fork of iai

@Ethan-000 Ethan-000 marked this pull request as ready for review August 8, 2023 21:27
@Ethan-000 Ethan-000 mentioned this pull request Aug 9, 2023
Merged
2 tasks
@Ethan-000
Copy link
Contributor Author

huh resolve can close pr too didn't know this

@Ethan-000 Ethan-000 reopened this Aug 9, 2023
@Ethan-000
Copy link
Contributor Author

removed bench ci

@kevaundray
Copy link
Contributor

This has turned into just adding basic benchmarking, this just means that we will need to manually run cargo bench, before and after the PR to see if there is a performance regression which is okay

@kevaundray kevaundray added this pull request to the merge queue Aug 11, 2023
Merged via the queue into master with commit c8fe617 Aug 11, 2023
17 checks passed
@kevaundray kevaundray deleted the e/bench branch August 11, 2023 17:41
@kevaundray kevaundray mentioned this pull request Aug 11, 2023
Merged
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into remove-keys-from-artifact
d9236f4 
* master:
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into abi-wasm
075fde8 
* master: (23 commits)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  feat: Include struct names in ABIs (#2266)
  feat(nargo): Add `--exact` flag to `nargo test` (#2272)
  fix: Fix assignment when both `mut` and `&mut` are used (#2264)
  feat: Add `assert_constant` (#2242)
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  ...
TomAFrench added a commit that referenced this pull request Aug 15, 2023
@TomAFrench
Merge branch 'master' into intrinsic-die
70aaf04 
* master: (26 commits)
  chore(noir): Release 0.10.0 (#2039)
  fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
  fix: Prevent panic when passing relative paths to `--program-dir` (#2324)
  fix: Overflowing assignment will result in an error (#2321)
  chore: clippy fixes (#2320)
  chore: Parameterize the build mode for noir-wasm (#2317)
  chore: Make `wasm` tests pull from `result` directory (#2319)
  chore: Fix typo (#2315)
  chore: Reuse workspace target directory in wasm build script (#2312)
  feat(nargo): Add `--workspace` flag to run commands in every package (#2313)
  chore(frontend): Replace `ModuleOrigin` with `Location` on `ModuleData` (#2308)
  fix: Fix 3 parser test cases in parsing (#2284)
  fix: Require package names to be non-empty (#2293)
  fix(nargo)!: Remove `-p` short flag from the `--program-dir` flag (#2300)
  feat: optionally output a debug artifact on compile (#2260)
  chore: `nargo info` now prints information as a prettified table  (#2282)
  fix(lsp): Pass `--program-dir` to test command from codelens (#2292)
  fix(nargo): Allow `--program-dir` flag anywhere in a command (#2290)
  feat: Execute brillig opcodes with constant inputs at compile-time (#2190)
  feat: Add basic benchmarking (#2213)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevaundray kevaundray kevaundray 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.
2 participants
@Ethan-000 @kevaundray