chore(opt): Memoize Type::canonicalize

[jfecher]

Description

Problem*

Resolves #8545

Summary*

Optimizes Type::canonicalize by remembering the returned result which prevents this function from being quadratic in input size since we're no longer re-doing work. This also required a small fix to keep the arithmetic_generics test working: we can't do if let Type::InfixExpr(..) = ... since that doesn't match on Type::CheckedCast variants which are meant to be transparent. With this fix that was previously causing canonicalize to leave some terms unsimplified.

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.

[asterite]

Nice!

I thought the solution could be memoizing but I didn't know if it was possible because types don't have IDs, though they do have equality and has so I guess it works.

It might be nice to add bench-show to see if there's any noticeable improvements in existing projects (just out of curiosity).

[asterite]

Um, I approved but I see there are some regressions, so maybe this broke something. In any case the changes look good.

[jfecher]

Um, I approved but I see there are some regressions, so maybe this broke something. In any case the changes look good.

Yeah, quite large ones 😅. I'll look into those before merging. Not sure what they are currently. The main difference I can think of is that some Type::CheckedCast may be different but those should be checked at monomorphization time rather than runtime IIRC.

[github-actions]

⚠️ 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: d64f0a7	Previous: d3cb2b2	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	2 s	1 s	2
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	3 s	1 s	3
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	2 s	1 s	2
test_report_zkpassport_noir_rsa_	3 s	2 s	1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

[jfecher]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

This makes me think it is just because I didn't merge master originally.

[github-actions]

🚀 Deployed on https://6840a4eed48f9000b4ca2509--noir-docs.netlify.app

[michaeljklein]

Is it possible that recursing to find an InfixExpr here and/or in try_unify_by_moving_constant_terms ends up removing CheckedCast's?

[jfecher]

Yes - although without doing so we were failing in the arithmetic_generics test from failing to catch this case for results which were now cached to include CheckedCast where they previously dropped it

[TomAFrench]

simplifying serialize a little bit, if we have the program

fn main() {
    let x = (1, [2, 3, 4]);
    assert_eq(x.serialize().len(), 4);
}

We get the initial SSA

acir(inline) fn main f0 {
  b0():
    v3 = make_array [Field 2, Field 3, Field 4] : [Field; 3]
    v6 = call f1(Field 1, v3) -> [Field; 2]   <---- Note we're expecting an array of size 2
    constrain u1 0 == u1 1
    return
}

If I change the tuple so that both elements are the same size, e.g.

fn main() {
    let x = ([1, 2, 3], [2, 3, 4]);
    assert_eq(x.serialize().len(), 4);
}

We now get the SSA

acir(inline) fn main f0 {
  b0():
    v3 = make_array [Field 1, Field 2, Field 3] : [Field; 3]
    v5 = make_array [Field 2, Field 3, Field 4] : [Field; 3]
    v7 = call f1(v3, v5) -> [Field; 6] <---- Note we're expecting an array of size 6
    constrain u1 0 == u1 1
    return
}

It seems like we're not distinguishing between the associated constants on the various subtypes making up the tuple and instead are applying the Size constant from the first element onto the rest of the tuple's elements.

[github-actions]

FYI @noir-lang/developerrelations on Noir doc changes.

[jfecher]

Closing this - #8826 should be a valid substitute.
The original issue is compiling faster but has errors during compilation (already captured in #8797). We can revisit/close it once the unrelated errors are fixed.