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

Overhaul `syntax::fold::Folder`. #58061

Merged
merged 10 commits into from Feb 6, 2019

Conversation

Projects
None yet
6 participants
@nnethercote
Copy link
Contributor

nnethercote commented Feb 1, 2019

This PR changes syntax::fold::Folder from a functional style
(where most methods take a T and produce a new T) to a more
imperative style (where most methods take and modify a &mut T), and
renames it syntax::mut_visit::MutVisitor.

This makes the code faster and more concise.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 1, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 1, 2019

⌛️ Trying commit b4a6b98 with merge 4d27e36...

bors added a commit that referenced this pull request Feb 1, 2019

Auto merge of #58061 - nnethercote:overhaul-syntax-Folder, r=<try>
Overhaul `syntax::fold::Folder`.

This isn't ready for review yet, but I want to do a perf run to see if the instruction reductions on CI match what I see locally (1--4% across many benchmarks and workloads).

r? @nnethercote (so nobody else is prematurely chosen as a reviewer).
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 1, 2019

@nnethercote
Hmm, have you seen #57662?

@petrochenkov petrochenkov self-assigned this Feb 1, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 2, 2019

☀️ Test successful - checks-travis
State: approved= try=True

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

I hadn't seen #57662. It's definitely overlapping with this PR. Notable differences, judging from a quick skim:

  • This PR changes Folder rather than introducing a new kind of AST walker. Therefore it has changes for all the existing Folder impls.
  • This PR has clear perf improvements (assuming the CI measurements match my local measurements).
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

The reason the new approach is faster is that an existing noop fold overwrites all the nodes with the same values. There are no reallocations due to the use of P::map and move_map, move_flat_map, but quite a lot of unnecessary writes. The new approach only overwrites data that needs overwriting, which for a typical pass is only a subset.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 2, 2019

Success: Queued 4d27e36 with parent 23d8d0c, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 2, 2019

Finished benchmarking try commit 4d27e36

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

The perf results are similar to what I saw locally. Widespread improvements, mostly on "clean incremental" runs, with a best result of -3.4%.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

Comparing this PR with #57662 some more, the handling of the T -> T cases is very similar. For example, noop_mod_expr in this PR is very similar to walk_expr in #57662 (though the former's use of destructuring avoids the need for all those ref mut patterns).

The handling of the other cases is where the differences are bigger. This PR keeps the existing structure for ones like fold_opt_expr (P<Expr> -> Option<P<Expr>>) and fold_stmt (Stmt -> SmallVec<[Stmt; 1]>); this minimizes change and means move_flat_map can still be used. (I did some ad hoc profiling with eprintln! and found that these non-T -> T cases are not hot, so keeping the existing structure seems ok.)

In contrast, #57662 introduces the Action type and a bunch of related machinery to handle these cases. It also uses the new walk_list/walk_list_mut macros instead of loops and/or move_flat_map to handle sequences.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 2, 2019

A final summary!

This PR:

  • Modifies the existing AST visitor, Folder.
  • Modifies the handling of all the easy cases (T -> T) in Folder.
  • Leaves the tricky cases (non-T -> T) unchanged.
  • Uses loops and the existing move_flat_map to handle sequences.
  • Updates all the existing folders, and therefore reaps the full performance benefits.
  • Overall, the number of lines of code is barely changed.

#57662:

  • Adds a new AST visitor, MutVisitor.
  • Handles the easy cases in much the same way as in this PR.
  • Introduces a new way of handling the tricky case (via the Action type).
  • Doesn't handle some cases yet (e.g. visit_interpolated).
  • Uses the new walk_list/walk_list_mut macros to handle sequences.
  • Updates two of the existing folders, and therefore doesn't yet get the full performance benefits.
  • Adds a lot of new lines of code. Eventually, Folder should be removable, but the other added machinery (Action, etc.) will likely still result in an increase in code size.

Overall, I think this PR is more complete while involving less changes. But I'm biased :) Let me know if I've gotten any of the details wrong.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 4, 2019

I have updated the code. It's now complete except for one thing: as the "njn:" comment in the code says, I think the syntax::fold::Folder name should change, though I'm not entirely sure what to. Suggestions are welcome.

r? @petrochenkov: the code is ready to review, but as well as making the main change, I have made a bunch of minor clean-ups. If you like I could pull out those clean-ups into separate commits preceding the main commit.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 4, 2019

Great work, thanks.
I see both this PR and #57662 as intermediate points to the end goal, but going through different trajectories.

I think I prefer this PRs trajectory. Why:

  • We can measure performance effects from the "noop folder vs noop visitor" and "tweaks to 1 -> many transformations" separately.
  • This PR is more "mechanical" and is based on the current Folder rather than Visitor, so it is more likely to preserve the exact behavior (bug compatibility, if you wish), e.g. around nonterminal folding.
    I'm not entirely happy about some parts of Folder and after this PR they'll still be here and I'll actually be able to fix them, rather than have a "devil we don't know" after #57662.
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 4, 2019

Regarding naming.
For 1 -> 1 transformations I think we should take the terminology from #57662 - MutVisitor/visit_thing (especially considering #57662 (comment)).
For 1 -> many I think we should keep exactly what we have now (fold_thing/move_map/...) and don't rename anything, since it's an intermediate step anyway.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 4, 2019

@nnethercote

If you like I could pull out those clean-ups into separate commits preceding the main commit.

If that's not too annoying, could you split the PR into two commits

  • The part containing only mechanical changes to definitions fn fold_thing(&mut self, thing: Thing) -> Thing => fn visit_thing(&mut self, thing: &mut Thing) and minimal changes to their uses required for things to compile again.
  • Everything else (cleanups, moving code around, moving comments around, ...).

@nnethercote nnethercote force-pushed the nnethercote:overhaul-syntax-Folder branch from d05b178 to f9d6e3e Feb 5, 2019

nnethercote added some commits Feb 5, 2019

Streamline `Folder`.
Specifically:

- Remove dead methods: fold_usize, fold_meta_items, fold_opt_bounds.

- Remove useless methods: fold_global_asm, fold_range_end.

- Inline and remove unnecessary methods: fold_item_simple,
  fold_foreign_item_simple.
Simplify `fold_attribute`.
It doesn't need to return an `Option`.

nnethercote added some commits Feb 5, 2019

Streamline `Folder` some more.
By eliminating some unnecessary methods, and moving/renaming some
functions that look like they might be methods but aren't.

@nnethercote nnethercote force-pushed the nnethercote:overhaul-syntax-Folder branch from f9d6e3e to 3263f02 Feb 5, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 5, 2019

@petrochenkov: the new code is up.

  • I split out 8 clean-up patches, because that's what I would have wanted if I was the reviewer. Should make your job easier.
  • I switched to mut_visit::MutVisitor. (I feel like it should be visit_mut::VisitorMut to match IterMut, but there are already a couple of existing MutVisitor occurrences in the compiler.)
  • I kept the flat_map_t and filter_map_t names, because I think they make the code easier to understand than fold_t names. Also, you say this is an intermediate step, but intermediate steps sometimes hang around longer than people expect...
  • git unfortunately didn't treat the renaming of fold.rs as mut_visit.rs as a move. I guess the two files are too dissimilar. Sorry, I don't know how to make that better.
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 5, 2019

@nnethercote

git unfortunately didn't treat the renaming of fold.rs as mut_visit.rs as a move.

The usual solution is to move the file renaming into a separate commit.
(Note that the Rust module can still be named mut_visit even if the file is named fold.rs.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 5, 2019

r=me with the file renaming moved into a separate commit or without it.

nnethercote added some commits Feb 5, 2019

Overhaul `syntax::fold::Folder`.
This commit changes `syntax::fold::Folder` from a functional style
(where most methods take a `T` and produce a new `T`) to a more
imperative style (where most methods take and modify a `&mut T`), and
renames it `syntax::mut_visit::MutVisitor`.

The first benefit is speed. The functional style does not require any
reallocations, due to the use of `P::map` and
`MoveMap::move_{,flat_}map`. However, every field in the AST must be
overwritten; even those fields that are unchanged are overwritten with
the same value. This causes a lot of unnecessary memory writes. The
imperative style reduces instruction counts by 1--3% across a wide range
of workloads, particularly incremental workloads.

The second benefit is conciseness; the imperative style is usually more
concise. E.g. compare the old functional style:
```
fn fold_abc(&mut self, abc: ABC) {
    ABC {
        a: fold_a(abc.a),
        b: fold_b(abc.b),
        c: abc.c,
    }
}
```
with the imperative style:
```
fn visit_abc(&mut self, ABC { a, b, c: _ }: &mut ABC) {
    visit_a(a);
    visit_b(b);
}
```
(The reductions get larger in more complex examples.)

Overall, the patch removes over 200 lines of code -- even though the new
code has more comments -- and a lot of the remaining lines have fewer
characters.

Some notes:

- The old style used methods called `fold_*`. The new style mostly uses
  methods called `visit_*`, but there are a few methods that map a `T`
  to something other than a `T`, which are called `flat_map_*` (`T` maps
  to multiple `T`s) or `filter_map_*` (`T` maps to 0 or 1 `T`s).

- `move_map.rs`/`MoveMap`/`move_map`/`move_flat_map` are renamed
  `map_in_place.rs`/`MapInPlace`/`map_in_place`/`flat_map_in_place` to
  reflect their slightly changed signatures.

- Although this commit renames the `fold` module as `mut_visit`, it
  keeps it in the `fold.rs` file, so as not to confuse git. The next
  commit will rename the file.

@nnethercote nnethercote force-pushed the nnethercote:overhaul-syntax-Folder branch from 3263f02 to bfcbd23 Feb 5, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Feb 5, 2019

The usual solution is to move the file renaming into a separate commit.

Thank you for the suggestion, I did that.

@bors r=petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2019

📌 Commit bfcbd23 has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 6, 2019

⌛️ Testing commit bfcbd23 with merge 2596bc1...

bors added a commit that referenced this pull request Feb 6, 2019

Auto merge of #58061 - nnethercote:overhaul-syntax-Folder, r=petroche…
…nkov

Overhaul `syntax::fold::Folder`.

This PR changes `syntax::fold::Folder` from a functional style
(where most methods take a `T` and produce a new `T`) to a more
imperative style (where most methods take and modify a `&mut T`), and
renames it `syntax::mut_visit::MutVisitor`.

This makes the code faster and more concise.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: petrochenkov
Pushing 2596bc1 to master...

@bors bors merged commit bfcbd23 into rust-lang:master Feb 6, 2019

1 check passed

homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:overhaul-syntax-Folder branch Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.