Skip to content

Commit

Permalink
Overhaul syntax::fold::Folder.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
nnethercote committed Feb 5, 2019
1 parent 970b5d1 commit 9fcb165
Show file tree
Hide file tree
Showing 23 changed files with 1,417 additions and 1,516 deletions.
27 changes: 13 additions & 14 deletions src/librustc_allocator/expand.rs
Expand Up @@ -16,7 +16,7 @@ use syntax::{
expand::ExpansionConfig,
hygiene::{self, Mark, SyntaxContext},
},
fold::{self, Folder},
mut_visit::{self, MutVisitor},
parse::ParseSess,
ptr::P,
symbol::Symbol
Expand All @@ -28,18 +28,18 @@ use {AllocatorMethod, AllocatorTy, ALLOCATOR_METHODS};
pub fn modify(
sess: &ParseSess,
resolver: &mut dyn Resolver,
krate: Crate,
krate: &mut Crate,
crate_name: String,
handler: &rustc_errors::Handler,
) -> ast::Crate {
) {
ExpandAllocatorDirectives {
handler,
sess,
resolver,
found: false,
crate_name: Some(crate_name),
in_submod: -1, // -1 to account for the "root" module
}.fold_crate(krate)
}.visit_crate(krate);
}

struct ExpandAllocatorDirectives<'a> {
Expand All @@ -54,14 +54,14 @@ struct ExpandAllocatorDirectives<'a> {
in_submod: isize,
}

impl<'a> Folder for ExpandAllocatorDirectives<'a> {
fn fold_item(&mut self, item: P<Item>) -> SmallVec<[P<Item>; 1]> {
impl<'a> MutVisitor for ExpandAllocatorDirectives<'a> {
fn flat_map_item(&mut self, item: P<Item>) -> SmallVec<[P<Item>; 1]> {
debug!("in submodule {}", self.in_submod);

let name = if attr::contains_name(&item.attrs, "global_allocator") {
"global_allocator"
} else {
return fold::noop_fold_item(item, self);
return mut_visit::noop_flat_map_item(item, self);
};
match item.node {
ItemKind::Static(..) => {}
Expand Down Expand Up @@ -139,25 +139,24 @@ impl<'a> Folder for ExpandAllocatorDirectives<'a> {
let name = f.kind.fn_name("allocator_abi");
let allocator_abi = Ident::with_empty_ctxt(Symbol::gensym(&name));
let module = f.cx.item_mod(span, span, allocator_abi, Vec::new(), items);
let module = f.cx.monotonic_expander().fold_item(module).pop().unwrap();
let module = f.cx.monotonic_expander().flat_map_item(module).pop().unwrap();

// Return the item and new submodule
smallvec![item, module]
}

// If we enter a submodule, take note.
fn fold_mod(&mut self, m: Mod) -> Mod {
fn visit_mod(&mut self, m: &mut Mod) {
debug!("enter submodule");
self.in_submod += 1;
let ret = fold::noop_fold_mod(m, self);
mut_visit::noop_visit_mod(m, self);
self.in_submod -= 1;
debug!("exit submodule");
ret
}

// `fold_mac` is disabled by default. Enable it here.
fn fold_mac(&mut self, mac: Mac) -> Mac {
fold::noop_fold_mac(mac, self)
// `visit_mac` is disabled by default. Enable it here.
fn visit_mac(&mut self, mac: &mut Mac) {
mut_visit::noop_visit_mac(mac, self)
}
}

Expand Down
9 changes: 9 additions & 0 deletions src/librustc_data_structures/thin_vec.rs
Expand Up @@ -39,6 +39,15 @@ impl<T> ::std::ops::Deref for ThinVec<T> {
}
}

impl<T> ::std::ops::DerefMut for ThinVec<T> {
fn deref_mut(&mut self) -> &mut [T] {
match *self {
ThinVec(None) => &mut [],
ThinVec(Some(ref mut vec)) => vec,
}
}
}

impl<T> Extend<T> for ThinVec<T> {
fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
match *self {
Expand Down
12 changes: 6 additions & 6 deletions src/librustc_driver/driver.rs
Expand Up @@ -32,7 +32,7 @@ use rustc_typeck as typeck;
use syntax::{self, ast, attr, diagnostics, visit};
use syntax::early_buffered_lints::BufferedEarlyLint;
use syntax::ext::base::ExtCtxt;
use syntax::fold::Folder;
use syntax::mut_visit::MutVisitor;
use syntax::parse::{self, PResult};
use syntax::util::node_count::NodeCounter;
use syntax::util::lev_distance::find_best_match_for_name;
Expand Down Expand Up @@ -1000,12 +1000,12 @@ where
});
sess.profiler(|p| p.end_activity(ProfileCategory::Expansion));

krate = time(sess, "maybe building test harness", || {
time(sess, "maybe building test harness", || {
syntax::test::modify_for_testing(
&sess.parse_sess,
&mut resolver,
sess.opts.test,
krate,
&mut krate,
sess.diagnostic(),
&sess.features_untracked(),
)
Expand All @@ -1014,7 +1014,7 @@ where
// If we're actually rustdoc then there's no need to actually compile
// anything, so switch everything to just looping
if sess.opts.actually_rustdoc {
krate = ReplaceBodyWithLoop::new(sess).fold_crate(krate);
ReplaceBodyWithLoop::new(sess).visit_crate(&mut krate);
}

let (has_proc_macro_decls, has_global_allocator) = time(sess, "AST validation", || {
Expand Down Expand Up @@ -1045,11 +1045,11 @@ where

if has_global_allocator {
// Expand global allocators, which are treated as an in-tree proc macro
krate = time(sess, "creating allocators", || {
time(sess, "creating allocators", || {
allocator::expand::modify(
&sess.parse_sess,
&mut resolver,
krate,
&mut krate,
crate_name.to_string(),
sess.diagnostic(),
)
Expand Down
9 changes: 5 additions & 4 deletions src/librustc_driver/lib.rs
Expand Up @@ -870,9 +870,9 @@ impl<'a> CompilerCalls<'a> for RustcDefaultCalls {
control.after_hir_lowering.stop = Compilation::Stop;

control.after_parse.callback = box move |state| {
state.krate = Some(pretty::fold_crate(state.session,
state.krate.take().unwrap(),
ppm));
let mut krate = state.krate.take().unwrap();
pretty::visit_crate(state.session, &mut krate, ppm);
state.krate = Some(krate);
};
control.after_hir_lowering.callback = box move |state| {
pretty::print_after_hir_lowering(state.session,
Expand All @@ -891,7 +891,8 @@ impl<'a> CompilerCalls<'a> for RustcDefaultCalls {
control.after_parse.stop = Compilation::Stop;

control.after_parse.callback = box move |state| {
let krate = pretty::fold_crate(state.session, state.krate.take().unwrap(), ppm);
let mut krate = state.krate.take().unwrap();
pretty::visit_crate(state.session, &mut krate, ppm);
pretty::print_after_parsing(state.session,
state.input,
&krate,
Expand Down
42 changes: 20 additions & 22 deletions src/librustc_driver/pretty.rs
Expand Up @@ -16,7 +16,7 @@ use rustc_metadata::cstore::CStore;
use rustc_mir::util::{write_mir_pretty, write_mir_graphviz};

use syntax::ast::{self, BlockCheckMode};
use syntax::fold::{self, Folder};
use syntax::mut_visit::{*, MutVisitor, visit_clobber};
use syntax::print::{pprust};
use syntax::print::pprust::PrintState;
use syntax::ptr::P;
Expand All @@ -28,6 +28,7 @@ use smallvec::SmallVec;
use std::cell::Cell;
use std::fs::File;
use std::io::{self, Write};
use std::ops::DerefMut;
use std::option;
use std::path::Path;
use std::str::FromStr;
Expand Down Expand Up @@ -703,42 +704,42 @@ impl<'a> ReplaceBodyWithLoop<'a> {
}
}

impl<'a> fold::Folder for ReplaceBodyWithLoop<'a> {
fn fold_item_kind(&mut self, i: ast::ItemKind) -> ast::ItemKind {
impl<'a> MutVisitor for ReplaceBodyWithLoop<'a> {
fn visit_item_kind(&mut self, i: &mut ast::ItemKind) {
let is_const = match i {
ast::ItemKind::Static(..) | ast::ItemKind::Const(..) => true,
ast::ItemKind::Fn(ref decl, ref header, _, _) =>
header.constness.node == ast::Constness::Const || Self::should_ignore_fn(decl),
_ => false,
};
self.run(is_const, |s| fold::noop_fold_item_kind(i, s))
self.run(is_const, |s| noop_visit_item_kind(i, s))
}

fn fold_trait_item(&mut self, i: ast::TraitItem) -> SmallVec<[ast::TraitItem; 1]> {
fn flat_map_trait_item(&mut self, i: ast::TraitItem) -> SmallVec<[ast::TraitItem; 1]> {
let is_const = match i.node {
ast::TraitItemKind::Const(..) => true,
ast::TraitItemKind::Method(ast::MethodSig { ref decl, ref header, .. }, _) =>
header.constness.node == ast::Constness::Const || Self::should_ignore_fn(decl),
_ => false,
};
self.run(is_const, |s| fold::noop_fold_trait_item(i, s))
self.run(is_const, |s| noop_flat_map_trait_item(i, s))
}

fn fold_impl_item(&mut self, i: ast::ImplItem) -> SmallVec<[ast::ImplItem; 1]> {
fn flat_map_impl_item(&mut self, i: ast::ImplItem) -> SmallVec<[ast::ImplItem; 1]> {
let is_const = match i.node {
ast::ImplItemKind::Const(..) => true,
ast::ImplItemKind::Method(ast::MethodSig { ref decl, ref header, .. }, _) =>
header.constness.node == ast::Constness::Const || Self::should_ignore_fn(decl),
_ => false,
};
self.run(is_const, |s| fold::noop_fold_impl_item(i, s))
self.run(is_const, |s| noop_flat_map_impl_item(i, s))
}

fn fold_anon_const(&mut self, c: ast::AnonConst) -> ast::AnonConst {
self.run(true, |s| fold::noop_fold_anon_const(c, s))
fn visit_anon_const(&mut self, c: &mut ast::AnonConst) {
self.run(true, |s| noop_visit_anon_const(c, s))
}

fn fold_block(&mut self, b: P<ast::Block>) -> P<ast::Block> {
fn visit_block(&mut self, b: &mut P<ast::Block>) {
fn stmt_to_block(rules: ast::BlockCheckMode,
s: Option<ast::Stmt>,
sess: &Session) -> ast::Block {
Expand Down Expand Up @@ -780,14 +781,14 @@ impl<'a> fold::Folder for ReplaceBodyWithLoop<'a> {
};

if self.within_static_or_const {
fold::noop_fold_block(b, self)
noop_visit_block(b, self)
} else {
b.map(|b| {
visit_clobber(b.deref_mut(), |b| {
let mut stmts = vec![];
for s in b.stmts {
let old_blocks = self.nested_blocks.replace(vec![]);

stmts.extend(self.fold_stmt(s).into_iter().filter(|s| s.is_item()));
stmts.extend(self.flat_map_stmt(s).into_iter().filter(|s| s.is_item()));

// we put a Some in there earlier with that replace(), so this is valid
let new_blocks = self.nested_blocks.take().unwrap();
Expand Down Expand Up @@ -818,9 +819,9 @@ impl<'a> fold::Folder for ReplaceBodyWithLoop<'a> {
}

// in general the pretty printer processes unexpanded code, so
// we override the default `fold_mac` method which panics.
fn fold_mac(&mut self, mac: ast::Mac) -> ast::Mac {
fold::noop_fold_mac(mac, self)
// we override the default `visit_mac` method which panics.
fn visit_mac(&mut self, mac: &mut ast::Mac) {
noop_visit_mac(mac, self)
}
}

Expand Down Expand Up @@ -889,12 +890,9 @@ fn print_flowgraph<'a, 'tcx, W: Write>(variants: Vec<borrowck_dot::Variant>,
}
}

pub fn fold_crate(sess: &Session, krate: ast::Crate, ppm: PpMode) -> ast::Crate {
pub fn visit_crate(sess: &Session, krate: &mut ast::Crate, ppm: PpMode) {
if let PpmSource(PpmEveryBodyLoops) = ppm {
let mut fold = ReplaceBodyWithLoop::new(sess);
fold.fold_crate(krate)
} else {
krate
ReplaceBodyWithLoop::new(sess).visit_crate(krate);
}
}

Expand Down

0 comments on commit 9fcb165

Please sign in to comment.