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.

Sign up for GitHub

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

Jump to bottom

Hasten macro parsing #68848

Closed
wants to merge 4 commits into from
Closed

Hasten macro parsing #68848

wants to merge 4 commits into from

Conversation

nnethercote
Copy link
Contributor

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2020
@nnethercote
Copy link
Contributor Author

This is a hacky but easy change that gets some of the wins that #68836 would get.

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 5, 2020

⌛ Trying commit e30f495ecec1a30ac97215bee2107b4fdaa1a884 with merge 0c7ea93de45e21ba54a804998b0b7563386ba795...

@Centril
Copy link
Contributor

Centril commented Feb 5, 2020

cc @petrochenkov

@petrochenkov petrochenkov self-assigned this Feb 5, 2020
@bors
Copy link
Contributor

bors commented Feb 5, 2020

☀️ Try build successful - checks-azure
Build commit: 0c7ea93de45e21ba54a804998b0b7563386ba795 (0c7ea93de45e21ba54a804998b0b7563386ba795)

@rust-timer
Copy link
Collaborator

Queued 0c7ea93de45e21ba54a804998b0b7563386ba795 with parent 002287d, future comparison URL.

@nnethercote
Copy link
Contributor Author

As expected, the perf results are excellent for html5ever and are negligible for all other benchmarks.

@petrochenkov
Copy link
Contributor

I like this, the idea is pretty simple and I wouldn't even call it a hack.
I've left one comment.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2020
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2020
petrochenkov
petrochenkov reviewed Feb 8, 2020
View reviewed changes
src/librustc_expand/lib.rs Outdated Show resolved Hide resolved
src/librustc_expand/mbe/macro_rules.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

The Directory change is not obvious to me.
Even if we now clone parser, the directory cloning is

  • still needed much rarer than the parser cloning,
  • parser cloning is more expensive if there's an owned directory inside it.

The change is justified if the directory cloning is so relatively cheap that it is lost in the noise from the whole parser cloning.
Have you measured the effect?
(Otherwise, I'd make this change because it makes code simpler.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2020
@nnethercote
Copy link
Contributor Author

Parser cloning is very rare on this code path, so much so that there is no measurable benefit to making it cheaper.

I previously made Directory a Cow to avoid allocations on this hot path. This new Parser-is-Cow optimization subsumes the old Directory-is-Cow optimization.

Does that help? I'm not sure how else to explain it.

@nnethercote
Copy link
Contributor Author

@petrochenkov are you happy to r+ this instead of @eddyb?

@petrochenkov
Copy link
Contributor

Yes, r? @petrochenkov

I left a couple of comments, r=me after addressing them.

@nnethercote
Avoid instantiating many Parser structs in generic_extension.
67da45f 
Currently, every iteration of the main loop in `generic_extension`
instantiates a `Parser`, which is expensive because `Parser` is a large
type. Many of those instantiations are only used immutably, particularly
for simple-but-repetitive macros of the sort seen in `html5ever` and PR
68836.

This commit initializes a single parser outside the loop, and then uses
`Cow` to avoid cloning it except for the mutating iterations. This
speeds up `html5ever` runs by up to 15%.
@nnethercote
Remove the Cow from Directory.
63bc4b3 
The previous commit wrapped `Parser` within a `Cow` for the hot macro
parsing path. As a result, there's no need for the `Cow` within
`Directory`, because it lies within `Parser`.
@nnethercote
Change condition ordering in parse_tt.
e6e6bb5 
This is a small win, because `Failure` is much more common than
`Success`.
@nnethercote
Fix a typo in a variable name.
6851f9a
@nnethercote
Copy link
Contributor Author

nnethercote commented Feb 10, 2020 •
edited
Loading

I addressed the comments, and added a trivial commit fixing a typo in a variable name.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Feb 10, 2020

📌 Commit 2a13b24 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2020
@jonas-schievink jonas-schievink mentioned this pull request Feb 11, 2020
Closed
@jonas-schievink jonas-schievink mentioned this pull request Feb 11, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 12, 2020
@Dylan-DPC
Rollup merge of rust-lang#68848 - nnethercote:hasten-macro-parsing, r…
8f813b7 
…=petrochenkov

Hasten macro parsing

r? @eddyb
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 12, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 13, 2020
@Dylan-DPC
Rollup merge of rust-lang#68848 - nnethercote:hasten-macro-parsing, r…
87ba8f2 
…=petrochenkov

Hasten macro parsing

r? @eddyb
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 13, 2020
Merged
bors added a commit that referenced this pull request Feb 13, 2020
@bors
Auto merge of #69118 - Dylan-DPC:rollup-7hpm1fj, r=Dylan-DPC
2e6eace 
Rollup of 9 pull requests

Successful merges:

 - #67642 (Relax bounds on HashMap/HashSet)
 - #68848 (Hasten macro parsing)
 - #69008 (Properly use parent generics for opaque types)
 - #69048 (Suggestion when encountering assoc types from hrtb)
 - #69049 (Optimize image sizes)
 - #69050 (Micro-optimize the heck out of LEB128 reading and writing.)
 - #69068 (Make the SGX arg cleanup implementation a NOP)
 - #69082 (When expecting `BoxFuture` and using `async {}`, suggest `Box::pin`)
 - #69104 (bootstrap: Configure cmake when building sanitizer runtimes)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Feb 13, 2020

☔ The latest upstream changes (presumably #69118) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 13, 2020
@nnethercote
Copy link
Contributor Author

Something weird happened here. Somehow an old version of this branch got merged. I will have to put the orphaned changes in a separate PR.

@nnethercote nnethercote deleted the hasten-macro-parsing branch February 13, 2020 23:52
@nnethercote nnethercote mentioned this pull request Feb 14, 2020
Merged
@nnethercote
Copy link
Contributor Author

Something weird happened here. Somehow an old version of this branch got merged. I will have to put the orphaned changes in a separate PR.

#69150 is the PR.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 14, 2020
@JohnTitor
Rollup merge of rust-lang#69150 - nnethercote:68848-follow-up, r=petr…
72def9a 
…ochenkov

Follow-up to rust-lang#68848

This PR contains some late changes to rust-lang#68848 that somehow didn't get included when that PR was merged in a roll-up.

r? @petrochenkov
@JohnTitor JohnTitor mentioned this pull request Feb 14, 2020
Merged
bors added a commit that referenced this pull request Feb 14, 2020
@bors
Auto merge of #69172 - JohnTitor:rollup-6cbmwcw, r=JohnTitor
b92c6ee 
Rollup of 7 pull requests

Successful merges:

 - #68129 (Correct inference of primitive operand type behind binary operation)
 - #68475 (Use a `ParamEnvAnd<Predicate>` for caching in `ObligationForest`)
 - #68856 (typeck: clarify def_bm adjustments & add tests for or-patterns)
 - #69051 (simplify_try: address some of eddyb's comments)
 - #69128 (Fix extra subslice lowering)
 - #69150 (Follow-up to #68848)
 - #69164 (Update pulldown-cmark dependency)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rust-timer @bors @Centril @petrochenkov @eddyb @rust-highfive