Use depth-first traversal for macro parsing - #159808
Conversation
`has_no_remaining_items_for_step()` is only meaningful for breadth-first traversal during parsing; it needs to be removed or adjusted to make depth-first traversal possible. This has a small effect on error-reporting behavior. In one case in the `assert-trailing-junk.rs` test, `$(,)` (at the end of the LHS) is matched against `blah`. This leads to three possible `MatcherLoc`s being investigated in the last step: the sequence start, the `,`, and EOF (in this order). Before this commit, the sequence start is reporting as `remaining_matcher`; when it is reached, there is nothing else in `cur_mps`. The `,` is ignored because the EOF is also in `cur_mps`, and the EOF is ignored because it is EOF. After this commit, the `,` is reported as `remaining_matcher`. This is the only change, and I think the new behavior is more useful; it will prioritize the contents of a sequence over a "sequence start" `MatcherLoc`. The `Display` impl for `MatcherLoc` incorrectly noted that "sequence start" is not used in diagnostics. With this change, it is less likely to be used, but I've left in a FIXME to investigate that thoroughly.
This commit reduces `Tracker`'s reliance on `Parser`. `Parser` can only be relied on for information about the furthest match; this is not a problem for BFS because all mps are at the same input position. But in DFS, mps will have varying input positions. Now the diagnostics tracker will collect every token observed from the parser. In the next commit, this will be used in `ambiguity()`. `tests/ui/macros` passes.
With this commit, `CollecTrackerAndEmitter::ambiguity()` relies on the `tokens` field (added in the last commit) instead of the `parser` parameter. It identifies ambiguity by finding the earliest position where ambiguity occurred. This nicely crosses the bridge from BFS to DFS -- in BFS, the position of ambiguity is obvious, but in DFS, there could be multiple ambiguities at different positions and the earliest one needs to be prioritized.
`check_for_ambiguity()` repeated some of the work done by `parse_tt()`
(specifically, processing mps from `cur_mps`). The previous flow for
metavar/EOF matching was:
- During `parse_tt_inner()`:
- During `match_one()`:
- Check for a match, e.g. with `nonterminal_may_begin_with()`.
- If `checking_for_ambiguity`, fail.
- Call `check_for_ambiguity()`:
- Drain everything in `cur_mps`.
- If anything matched successfully, fail.
- Finish processing the mp, e.g. `Parser::parse_nonterminal()`.
The new flow is:
- During `parse_tt_inner()`:
- During `match_one()`:
- Check for a match, e.g. with `nonterminal_may_begin_with()`.
- Store the mp in `maybe_ambig_mp`.
- If something is already there, fail.
- Drain everything in `cur_mps`.
- If `maybe_ambig_mp` is set:
- If anything matched successfully, fail.
- Finish processing the mp, e.g. `Parser::parse_nonterminal()`.
This is quite similar to the structure before I started making changes,
e.g. via `bb_mps`. In the new structure, it also handles EOF.
`tests/ui/macros` passes.
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @nnethercote (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use depth-first traversal for macro parsing
f2fc1eb to
5413c09
Compare
|
The bug was a bit more involved than I realized, but I managed to retain the old behavior (at least as far as the UI tests are concerned). In addition (the last two commits), I simplified the control flow around |
Previously, `remaining_matcher` was picked based on implementation dependent ordering of mps. With this commit, it is deterministically selected as the furthest-along mp with the furthest-along loc.
Needed so I can add another field to `MatcherPos` without growing it.
This will form the basis for backtracking. Note that it is cleared after meta-variable parsing; since meta-variables do not admit ambiguity, and `cur_mps` is cleared by `check_for_ambiguity()` when a meta-variable is about to be parsed, no mps could possibly exist to refer to older tokens from the input.
This will be used for backtracking. At the moment, all mps in `cur_mps` have the same input position, and all mps in `next_mps` have the same input position (exactly one more than that in `cur_mps`).
`tests/ui/macros` passes!!!
Instead of pushing and popping `backtrack` all the time, try to return an mp for immediate use.
5413c09 to
1859c23
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (cb2bbd4): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -3.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.4%, secondary 14.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.274s -> 487.61s (-0.14%) |
|
Oh, yeesh. |
|
Instruction counts are moderately bad, cycles and wall-times are significantly worse :( |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Use depth-first traversal for macro parsing
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ff100f1): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.3%, secondary 8.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 488.104s -> 488.233s (0.03%) |
|
A lot more tenable, but cycle counts are still regressing. I seem to be on the right track and I can think of some ways to improve it further :) |
|
Looking forward to the improved version :) @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
View all comments
This PR changes the control flow in rustc's declarative macro parser to use a depth-first traversal instead of a breadth-first traversal. It builds upon a series of preparatory changes (#158577, #158894, #158974, #158976) that removed reliance on breadth-first traversal (usually with some performance improvements).
There are several motivations for this change.
It introduces an important invariant (the sort order in
backtrack) needed for optimizingMatcherPos::matches(turning it into a single, non-Rc'd field onTtParser). This will be implemented in a later PR.It reduces reliance on the heap. The
backtrackstack is only accessed when an mp fails to match. While it introduces theseen_tokenslist, this list is kept quite small, and it can be optimized further (e.g. only adding tokens to it if!backtrack.is_empty()).It should offer a more predictable workflow for the CPU, possibly showing a greater cycle-count improvement than instruction-count.
This is a perf-sensitive PR, I'd appreciate it if someone can start a perf run.
r? @nnethercote. Commits are individually reviewable. I'm happy to split out some of the earlier commits into separate PRs.