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

Optimize MBE (macro by example) performance #7857

Open
edwin0cheng opened this issue Mar 3, 2021 · 10 comments
Open

Optimize MBE (macro by example) performance #7857

edwin0cheng opened this issue Mar 3, 2021 · 10 comments
Labels
A-macro macro expansion A-perf performance issues E-has-instructions Issue has some instructions and pointers to code to get started fun A technically challenging issue with high impact good first issue S-actionable Someone could pick this issue up and work on it right now

Comments

@edwin0cheng
Copy link
Member

edwin0cheng commented Mar 3, 2021

Recently (#7513) we implemented a new NFA based expander for MBE which is quite slow compared to the old one (based on recursive descent). It would be nice if someone could optimize it to run as fast as possible.

Benchmark

We already have a benchmark for it, to run the benchmark :

$ RUN_SLOW_TESTS=1 cargo test --release --package mbe -- benchmark::benchmark_expand_macro_rules --nocapture

Output should be something like:

running 1 test
mbe expand macro rules: 2.21s, 8464minstr
test benchmark::benchmark_expand_macro_rules ... ok

Source

The corresponding source code of mbe expander is at :

https://github.com/rust-analyzer/rust-analyzer/blob/3b507aa90fca9618ddbe0667e245ef4766aa96b5/crates/mbe/src/expander/matcher.rs#L148-L151

And we have a bunch of compliance tests related to mbe, to run these test :

$  cargo test --package mbe -- --nocapture

Tips

By default, RA turn off the debug information in Cargo.toml :

https://github.com/rust-analyzer/rust-analyzer/blob/3b507aa90fca9618ddbe0667e245ef4766aa96b5/Cargo.toml#L18-L20

You may need to set it to debug = 2
And here is how I run perf in linux:

$ RUN_SLOW_TESTS=1 perf record --call-graph dwarf  cargo test --release --package mbe -- benchmark::benchmark_expand_macro_rules --nocapture
$ perf report --call-graph

[EDIT: added --release flag]

@edwin0cheng edwin0cheng added A-macro macro expansion fun A technically challenging issue with high impact E-has-instructions Issue has some instructions and pointers to code to get started labels Mar 3, 2021
@edwin0cheng edwin0cheng added the S-actionable Someone could pick this issue up and work on it right now label Mar 3, 2021
@edwin0cheng
Copy link
Member Author

CC #5549

@jonas-schievink
Copy link
Contributor

It's probably better to run the benchmark with --release, otherwise it doesn't get optimized and you'll optimize for the wrong thing.

@edwin0cheng
Copy link
Member Author

Good point ! Edited for --release

@dzmitry-lahoda
Copy link

if this is root cause of #7934 , than it is on linux also, and really slow on diesel compilation (diesel is very very meta template heavy)

@flodiebold
Copy link
Member

See also #4186.

@TimoFreiberg
Copy link
Contributor

For me, the benchmark showed improvements when replacing most of the SmallVecs in match_loop and mach_loop_inner with Vecs. I don't know whether my local benchmarks are meaningful though...
Do you want me to create a PR for this? master...TimoFreiberg:7857-benchmark

For the benefit of other readers, here's a flamegraph from my machine.
It looks like cloning the MatchState is the most expensive operation...

@edwin0cheng
Copy link
Member Author

For the benefit of other readers, here's a flamegraph from my machine.
It looks like cloning the MatchState is the most expensive operation...

Yeah, and specially cloning for Bindings , I implemented #7994 for that, but in general it still needed to improve a lot.

@Veykril Veykril added the A-perf performance issues label Jan 17, 2022
bors added a commit that referenced this issue Sep 14, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Refactor macro-by-example code

I had a look at the MBE code because of #7857. I found some easy readability wins, that might also _marginally_ improve perf.
@novacrazy
Copy link

novacrazy commented Mar 24, 2023

What is the status of this? I've seen a couple other issues closed and redirecting here, so I'll just say this here.

I have a macro for a quick SQL DSL using embedded Rust syntax and data types, generated from a build-script with around 900 rules, and rust-analyzer absolutely chokes on it. Causes the CPU to spin for multiple minutes, on a single thread and over 10 minutes sometimes, and pushes CPU temperature high enough that I repasted my CPU because it was so concerning. It effectively disables RA for the entire function that macro is used within, at least for those minutes. rustc seems to have no issues compiling it.

Furthermore, after it eventually finishes, RA only recognizes Rust identifiers (with on-hover information) used for about 128 tokens (not exactly measured) into the macro, the rest are left unknown as generic syntax. The macro also defines a datatype which is returned indirectly though another struct's type-parameters and through a closure's return value, deduced via type-inference. RA cannot recognize methods that exist on this type, but does know the name of it and that it implements Deref. Derefed methods do autocomplete, just not methods implemented directly on it from within the macro.

If requested, I could provide a minimal crate using this macro as I do in my real project.

@novacrazy
Copy link

After adding more usages of this macro to my codebase, rust-analyzer may as well not be running at all sometimes. No autocomplete, suggestions (like to add a missing import) take upwards of 20 seconds, and it blocks files from saving sometimes until I kill the process as it stops responding inside VS Code. It's also consuming over 5GB of RAM. At this rate I'll have to find a way to port it to a proc-macro just to get a reasonable dev experience with RA.

@dzmitry-lahoda

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion A-perf performance issues E-has-instructions Issue has some instructions and pointers to code to get started fun A technically challenging issue with high impact good first issue S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

7 participants