Skip to content

NFA parser for mbe matcher #7513

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

Merged
merged 1 commit into from
Mar 2, 2021
Merged

Conversation

edwin0cheng
Copy link
Member

Almost straight porting from rustc one, but a little bit slow :(

rust-analyzer analysis-stats -q . 

From:

Database loaded:     636.11ms, 277minstr
  crates: 36, mods: 594, decls: 11527, fns: 9017
Item Collection:     10.99s, 60ginstr
  exprs: 249618, ??ty: 2699 (1%), ?ty: 2101 (0%), !ty: 932
Inference:           28.94s, 123ginstr
Total:               39.93s, 184ginstr

To:

Database loaded:     630.90ms, 277minstr
  crates: 36, mods: 594, decls: 11528, fns: 9018
Item Collection:     13.70s, 77ginstr
  exprs: 249482, ??ty: 2699 (1%), ?ty: 2101 (0%), !ty: 932
Inference:           30.27s, 133ginstr
Total:               43.97s, 211ginstr

Fixes #4777

@edwin0cheng edwin0cheng force-pushed the nfa-mbe-matcher branch 3 times, most recently from 30ca5bc to 18b7e21 Compare February 2, 2021 10:33
@edwin0cheng edwin0cheng force-pushed the nfa-mbe-matcher branch 2 times, most recently from 9c7dee8 to 3e5b65b Compare February 3, 2021 07:08
@matklad
Copy link
Member

matklad commented Feb 4, 2021

Yeah, the regression is sizable, but it's not like we can not do it. I think some of it can be clawed back by just profiling and microoptimiaton.

However, to do that, we really need a benchmark here. I ... now realize we don't have a great benchmarking story at all, but it's better to start somewhere.

@edwin0cheng, could you do a separate PR which includes some bencmarks? I think they should be just usual #[test] tests (with run_slow_test check) which just use StopWatch to observe timings. The bench should live in the mbe crate. The bench should show the same impact for this PR, but it should be much easier to run.

With the benchmark in place, and with a good-first-issuer "optimize this", I'll be more than comfortable to merge this.

bors bot added a commit that referenced this pull request Feb 27, 2021
7566: Add benchmark tests for mbe r=matklad a=edwin0cheng

This PR add more real world tests dumped from `rust-analyzer analysis-stats .` to benchmark its performance.

cc #7513

r? @matklad 

Co-authored-by: Edwin Cheng <[email protected]>
@edwin0cheng
Copy link
Member Author

edwin0cheng commented Feb 27, 2021

Rebased from #7566 and here is the benchmark result :

before:
mbe expand macro rules: 1.21s, 4966minstr

after
mbe expand macro rules: 2.24s, 8464minstr

2x slower :(

@edwin0cheng edwin0cheng force-pushed the nfa-mbe-matcher branch 2 times, most recently from 9d8af91 to 2473256 Compare February 27, 2021 22:44
@matklad
Copy link
Member

matklad commented Mar 2, 2021

bors r+

Could you create an issue with mentoring instructions on how to run the benchmark? I wonder if we can nerd-snipe someone to look at the code with a profiler...

@bors
Copy link
Contributor

bors bot commented Mar 2, 2021

@bors bors bot merged commit 91bf5fa into rust-lang:master Mar 2, 2021
@edwin0cheng edwin0cheng deleted the nfa-mbe-matcher branch March 3, 2021 04:58
@edwin0cheng
Copy link
Member Author

Could you create an issue with mentoring instructions on how to run the benchmark? I wonder if we can nerd-snipe someone to look at the code with a profiler...

#7857

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.

Incorrect truncated macro expansion
3 participants