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: adding benchmarking tools for number of brillig opcodes #144

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kashbrti
Copy link
Contributor

@kashbrti kashbrti commented Mar 4, 2025

Description

Problem*

Resolves

Summary*

Additional Context

PR Checklist*

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

@TomAFrench
Copy link
Member

Looks like you're not making use of --force-brillig here which would simplify things a lot.

@TomAFrench
Copy link
Member

See https://github.com/noir-lang/noir/blob/master/test_programs/gates_report_brillig.sh for some inspiration (although you can't make use of nargo info.)

@kashbrti
Copy link
Contributor Author

kashbrti commented Mar 4, 2025

should I do the --force-brillig when doing the nargo export?

@TomAFrench
Copy link
Member

Yeah you need to specify that you want brillig output when you compile.

@kashbrti
Copy link
Contributor Author

kashbrti commented Mar 5, 2025

@TomAFrench I'm wondering if the format of the outputs generated by the script is compatible with the noir_gate_diff action. I'm not finding documentation explaining the json format required by the gate_diff

@TomAFrench
Copy link
Member

https://github.com/noir-lang/noir-gates-diff/blob/dbe920a8dcc3370af4be4f702ca9cef29317bec1/src/types.ts#L9-L12

The action expects a JSON object {"name": string, "opcodes": number} for each brillig function we're tracking.

@TomAFrench
Copy link
Member

Ah, I see you changed to that. Will take a closer look.

@kashbrti
Copy link
Contributor Author

kashbrti commented Mar 5, 2025

ah then it should be good. The objects in the Json are called unconstrained_function instead of function though. should I change that or is that expected?

@TomAFrench
Copy link
Member

That's expected.

Comment on lines 32 to 34
with:
report: opcode_report.json
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be marked as a brillig report

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 To Triage
Development

Successfully merging this pull request may close these issues.

2 participants