-
Notifications
You must be signed in to change notification settings - Fork 242
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: early check type equality in try_unify #7263
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20
.
Benchmark suite | Current: ccf9c99 | Previous: fc75298 | Ratio |
---|---|---|---|
AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
62 s |
50 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Awesome, can we create a regression test out of that branch? I'm not sure how much we can just pull out the changes made in that branch vs the full contract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to double check the raw equality check on types worked as expected but it does and monomorphization relies on this as well to some degree.
I tried. Or, well, at least I tried with a program that had As a side note, I was curious if this generally improved compile times. In Aztec-Packages |
…_make_arrays` (noir-lang/noir#7264) chore: early check type equality in try_unify (noir-lang/noir#7263) feat(LSP): suggest enum variants without parameters (noir-lang/noir#7261) feat(experimental): Parse match expressions (noir-lang/noir#7243) feat(experimental): Implement zeroed for enums (noir-lang/noir#7252) fix(ssa): Only attempt to inline constant Brillig calls for entry points (noir-lang/noir#7260) fix: Add missing `is_empty` check for enums (noir-lang/noir#7257) feat(experimental): Implement enum tag constants (noir-lang/noir#7183) fix(unrolling): Fetch original bytecode size from the original function (noir-lang/noir#7253) fix(ssa): Use number of SSA instructions for the Brillig unrolling bytecode size limit (noir-lang/noir#7242) feat: Sync from aztec-packages (noir-lang/noir#7241) chore: bump gates diff (noir-lang/noir#7245) feat: simplify subtraction from self to return zero (noir-lang/noir#7189) feat: allow specifying multiple patterns in nargo test (noir-lang/noir#7186) fix: Avoid type error when calling something with a type alias of a function (noir-lang/noir#7239) feat: Allow resolved types in constructors (noir-lang/noir#7223) chore: clarify to_radix docs examples (noir-lang/noir#7230) chore: fix struct example (noir-lang/noir#7198) feat(optimization): Add purity analysis to SSA (noir-lang/noir#7197) chore: start tracking time to run critical library tests (noir-lang/noir#7221) chore: Rework defunctionalize pass to not rely on DFG bugs (noir-lang/noir#7222) fix(brillig): Globals entry point reachability analysis (noir-lang/noir#7188) chore: update docs to use devcontainer feature (noir-lang/noir#7206) chore(ci): Add test for global vars entry points regression (noir-lang/noir#7209) chore(ssa): Flip the SSA Brillig constraint check to off by default (noir-lang/noir#7211) chore(docs): moving references to noir-starter to awesome-noir (noir-lang/noir#7203) chore: build docs in the merge queue (noir-lang/noir#7218) fix: correct reversed callstacks (noir-lang/noir#7212) chore: exclude dependency fetching time from benchmarks (noir-lang/noir#7210) feat(experimental): Support enums in comptime code (noir-lang/noir#7194)
noir-lang/noir#7264) chore: early check type equality in try_unify (noir-lang/noir#7263) feat(LSP): suggest enum variants without parameters (noir-lang/noir#7261) feat(experimental): Parse match expressions (noir-lang/noir#7243) feat(experimental): Implement zeroed for enums (noir-lang/noir#7252) fix(ssa): Only attempt to inline constant Brillig calls for entry points (noir-lang/noir#7260) fix: Add missing `is_empty` check for enums (noir-lang/noir#7257) feat(experimental): Implement enum tag constants (noir-lang/noir#7183) fix(unrolling): Fetch original bytecode size from the original function (noir-lang/noir#7253) fix(ssa): Use number of SSA instructions for the Brillig unrolling bytecode size limit (noir-lang/noir#7242) feat: Sync from aztec-packages (noir-lang/noir#7241) chore: bump gates diff (noir-lang/noir#7245) feat: simplify subtraction from self to return zero (noir-lang/noir#7189) feat: allow specifying multiple patterns in nargo test (noir-lang/noir#7186) fix: Avoid type error when calling something with a type alias of a function (noir-lang/noir#7239) feat: Allow resolved types in constructors (noir-lang/noir#7223) chore: clarify to_radix docs examples (noir-lang/noir#7230) chore: fix struct example (noir-lang/noir#7198) feat(optimization): Add purity analysis to SSA (noir-lang/noir#7197) chore: start tracking time to run critical library tests (noir-lang/noir#7221) chore: Rework defunctionalize pass to not rely on DFG bugs (noir-lang/noir#7222) fix(brillig): Globals entry point reachability analysis (noir-lang/noir#7188) chore: update docs to use devcontainer feature (noir-lang/noir#7206) chore(ci): Add test for global vars entry points regression (noir-lang/noir#7209) chore(ssa): Flip the SSA Brillig constraint check to off by default (noir-lang/noir#7211) chore(docs): moving references to noir-starter to awesome-noir (noir-lang/noir#7203) chore: build docs in the merge queue (noir-lang/noir#7218) fix: correct reversed callstacks (noir-lang/noir#7212) chore: exclude dependency fetching time from benchmarks (noir-lang/noir#7210) feat(experimental): Support enums in comptime code (noir-lang/noir#7194)
Description
Problem
Resolves #7256
Summary
While debugging this I realized that
try_unify
was being called withself
andother
being complex infix expressions that were actually the same type (same type variables, etc.). When that happened canonicalizing and trying to unify both would involve a lot of computations (it ends up trying to unify lots ofu32
for a reason that I still didn't sit down to investigate). I tried doing an early equality check intry_unify
and, magically, the time to compile the relevant program went from 10 minutes to 3 seconds.(I also didn't investigate yet why the same two types end up being unified)
Additional Context
I think checking for equality here is fine because it means there's nothing to unify, but let me know if this is not okay.
The second commit is just a micro-optimization: when doing
if let (_, _) = (a, b)
,a
andb
are evaluated and then pattern-matched. Becauseevaluate_to_field_element
might be an expensive call this just ends up doing one of those calls if it ends up returningErr
.Documentation
Check one:
PR Checklist
cargo fmt
on default settings.