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

Replace tuple implementation macros with typle #614

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jongiddy
Copy link

I've been working on the typle crate to make tuple implementations look more like typical Rust code. I found chumsky had some complex macro-based tuple implementations, so I've been testing typle with these.

I thought you might like to see the results. This generates very similar code to the existing macro-base tuple implementations. The only intentional semantic difference is in Choice, where the input rewind is avoided after an error on the final choice for all tuple sizes. It is currently only avoided for a 1-tuple.

As well as being more readable than the macros, using typle allows rust-analyzer to parse the code to provide type insets:
image

@zesterer
Copy link
Owner

Interesting! I was a little suspicious at first (in a 'does it really help?' way rather than a 'assuming ill intent' way), but I definitely agree that the resultant code is simpler to read and most likely will be simpler to maintain.

That said, I do have a few questions:

  • What strategy for code generation does typle use? I see there's a for loop in there: is it unrolling the loop body during macro evaluation to ensure that the resulting code is guaranteed to still perform static dispatch on the tuple members?

  • How is typle implemented? My assumption is that it's using a procedural macro internally. I'm interested to know what potential overhead this could be adding to compilation times. chumsky can already exhibit some fairly pessimistic compilation times and I'd prefer to avoid adding to that too much if I can help it.

@jongiddy
Copy link
Author

typle builds contexts containing the type and index identifiers and then parses the item to find instances of the identifiers or the typle_* macros, replacing them with code snippets. The for loop with a typle_index! unrolls completely. It is mostly the same as the macro$()* form, except that it supports break and continue, hence the addition of the 'start label to the pratt module, and it allows the range of indexes to be specified, hence the ability to treat initial and final iterations differently, as in the primitive module.

You are right about the compilation times. I ran cargo build --timings and the build time went from 3.3 s to 5.7 s, with typle adding 0.7 s and chumsky increasing by 0.4 s. There is some low-hanging fruit for improving typle performance - I haven't worried about optimization at all yet - but the biggest cost is the addition of the syn crate. It was already present in the Cargo.lock file due to dev dependencies but bringing it into the main build is a real blocker.

@zesterer
Copy link
Owner

zesterer commented Mar 19, 2024

Thanks for the in-depth explanation. It's good to know that typle isn't going to meaningfully add runtime overhead to things.

As you say, the compilation times are a bit unfortunate: I'm in two minds about this PR, since it's clearly a win from a maintainability standpoint. That said, I'm unconvinced that it adds enough (or rather, removes enough complexity-wise) to justify the compilation time footprint, at least today. Chumsky is already quite a complicated crate internally, so a recursive macro like in the original code doesn't really pose much of an additional barrier for maintainers, at least compared to what's already there.

Out of interest, when you ran cargo build --timings, was that on a rebuild after a small change or a fresh build? The use-case we're trying to optimise for the the former, since it's pretty common to end up in a tweak-build-test-tweak cycle when writing parsers.

Assuming that there's a similar cost associated with rebuilds, I would like to keep this PR open pending further compile-time optimisations to typle (or the specific usage of it here). I feel that the combinators that have been changed in the diff are unlikely to change substantially in the future, so rebasing shouldn't be too much of a hassle if the situation changes and merging this PR seems like it hits the desired power-to-weight ratio.

This generates almost identical code to the existing macros while being
more readable.

The only semantic difference is for `Choice`, where the final option
does not rewind the input.
@jongiddy
Copy link
Author

All timings are on a fresh build (and specific to my laptop). As the generated code is very similar to the macro version, I do not expect any difference in compilation or runtime for crates that use chumsky. With typle v0.9.6 the chumsky crate build is now only 0.25 s longer than main, increasing from 2.06 s to 2.31 s.

The overall time for a fresh build is 5.3 s, still much longer than the 3.3 s of main. That 2 s comes from the serialized builds of quote, syn, and typle so is unlikely to decrease much further.

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.

None yet

2 participants