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: early check type equality in try_unify #7263

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

asterite
Copy link
Collaborator

@asterite asterite commented Feb 3, 2025

Description

Problem

Resolves #7256

Summary

While debugging this I realized that try_unify was being called with self and other 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 of u32 for a reason that I still didn't sit down to investigate). I tried doing an early equality check in try_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 and b are evaluated and then pattern-matched. Because evaluate_to_field_element might be an expensive call this just ends up doing one of those calls if it ends up returning Err.

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 github-actions bot left a 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

@TomAFrench
Copy link
Member

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.

Copy link
Contributor

@jfecher jfecher left a 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.

@jfecher jfecher added this pull request to the merge queue Feb 3, 2025
@asterite
Copy link
Collaborator Author

asterite commented Feb 3, 2025

I tried. Or, well, at least I tried with a program that had get_arr_of_size__log_bytes_padding__from_plaintext and all the necessary functions, called it in a similar way done there... but it ends up being fast. I still see a lot more work is done before this PR in that case, but I couldn't find a program that would take a lot of time. I'll try it in a follow-up PR.

As a side note, I was curious if this generally improved compile times. In Aztec-Packages master running nargo compile --force --skip-brillig-constraints-check --silence-warnings takes 46 seconds before this PR, and 19 seconds with this PR 🎉

Merged via the queue into master with commit 0b6c382 Feb 3, 2025
103 checks passed
@jfecher jfecher deleted the ab/try-unify-early-equality-check branch February 3, 2025 18:19
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 4, 2025
…_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)
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 4, 2025
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)
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.

Example of compile hanging forever with no output
3 participants