chore(parser): Parser error optimisation

[joss-aztec]

Related issue(s)

Resolves #

Description

Summary of changes

tl;dr
~40% speed up in release mode
~10% speed up in debug mode

Chumsky produces and aggregates large numbers of errors in normal operation regardless of whether it's on the happy path. This is because as it tries each grammar rule in turn, it produces an error until the matching rule is encountered. Currently our error construction and aggregation is rather expensive, largely in due to the construction and destruction of BTreeSets that may hold zero or very few items.

This PR uses SmallOrdSet to avoid heap allocations as long as possible.

Other changes:

• labels switched to an enum to avoid string allocations

Samples taken using time nargo print-acir

	debug	release
master	4.1s	0.47s
this branch	3.7s	0.34s

Dependency additions / changes

Test additions / changes

Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt with default settings.
• I have linked this PR to the issue(s) that it resolves.
• I have reviewed the changes on GitHub, line by line.
• I have ensured all changes are covered in the description.

Documentation needs

• This PR requires documentation updates when merged.

Additional context

[jfecher]

Looks pretty good, I appreciate the timing information as well. Have you tested against existing small set crates? There aren't many but I wonder it one of those fits our usecase any better.

[joss-aztec]

Just noticed these changes throw up the following clippy warning:

warning: the `Err`-variant returned from this function is very large
   --> crates/noirc_frontend/src/ast/mod.rs:154:10
    |
154 |     ) -> Result<UnresolvedTypeExpression, ParserError> {
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 344 bytes
    |
    = help: try reducing the size of `parser::errors::ParserError`, for example by boxing large elements or replacing it with `Box<parser::errors::ParserError>`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
    = note: `#[warn(clippy::result_large_err)]` on by default

Guess I better benchmark Resolver::resolve_expression to check I haven't inadvertently hurt it. If not, then I guess it's justifiable to mute the warning.

I'm aware that I've spent more time on this PR than it's really worth - so shout if you think I should abandon.

[jfecher]

I think this PR is still a worthwhile pursuit, its nice to have the timing information so that we know for later what any performance characteristics are like. I'm interested in what the performance of tinyset would be as well? Is it an easy slot-in replacement for small-ord-set?

[joss-aztec]

I had a go at tinyset, but it's pretty cumbersome to integrate with our current enums. You need to implement the following trait on your element type:

pub trait Fits64: Copy {
    /// Convert back *from* a u64.  This is unsafe, since it is only
    /// infallible (and lossless) if the `u64` originally came from
    /// type `Self`.
    unsafe fn from_u64(x: u64) -> Self;
    /// Convert to a `u64`.  This should be infallible.
    fn to_u64(self) -> u64;
}

Probably the biggest hurdle is you'd need to map to some alternative representation of a token that's able to support the Copy trait. (Maybe an argument to split data from ids as we have done for ssa?) I also had trouble with unsafe related errors, but that could be my rust inexperience rather than a real blocker.

[jfecher]

Looks good. I have one final question but I don't want to hold this PR any longer.