Skip to content

Commit

Permalink
Rollup merge of rust-lang#30863 - jseyfried:no_rc, r=eddyb
Browse files Browse the repository at this point in the history 
Use arena allocation instead of reference counting for `Module`s to fix memory leaks from `Rc` cycles.

A module references its module children and its import resolutions, and an import resolution references the module defining the imported name, so there is a cycle whenever a module imports something from an ancestor module.

For example,
```rust
mod foo { // `foo` references `bar`.
    fn baz() {}
    mod bar { // `bar` references the import.
        use foo::baz; // The import references `foo`.
    }
}
```
  • Loading branch information
@Manishearth
Manishearth committed Jan 14, 2016
2 parents e51de04 + a8514d3 commit 1246f43
Show file tree
Hide file tree
Showing 5 changed files with 260 additions and 245 deletions.
2 changes: 1 addition & 1 deletion mk/crates.mk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ DEPS_rustc_lint := rustc log syntax
DEPS_rustc_llvm := native:rustllvm libc std rustc_bitflags
DEPS_rustc_metadata := rustc rustc_front syntax rbml
DEPS_rustc_mir := rustc rustc_front syntax
DEPS_rustc_resolve := rustc rustc_front log syntax
DEPS_rustc_resolve := arena rustc rustc_front log syntax
DEPS_rustc_platform_intrinsics := rustc rustc_llvm
DEPS_rustc_plugin := rustc rustc_metadata syntax
DEPS_rustc_privacy := rustc rustc_front log syntax
Expand Down
112 changes: 54 additions & 58 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use Module;
use Namespace::{TypeNS, ValueNS};
use NameBindings;
use {names_to_string, module_to_string};
use ParentLink::{self, ModuleParentLink, BlockParentLink};
use ParentLink::{ModuleParentLink, BlockParentLink};
use Resolver;
use resolve_imports::Shadowable;
use {resolve_error, resolve_struct_error, ResolutionError};
Expand Down Expand Up @@ -52,7 +52,6 @@ use rustc_front::intravisit::{self, Visitor};

use std::mem::replace;
use std::ops::{Deref, DerefMut};
use std::rc::Rc;

// Specifies how duplicates should be handled when adding a child item if
// another item exists with the same name in some namespace.
Expand Down Expand Up @@ -86,7 +85,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
/// Constructs the reduced graph for the entire crate.
fn build_reduced_graph(self, krate: &hir::Crate) {
let mut visitor = BuildReducedGraphVisitor {
parent: self.graph_root.clone(),
parent: self.graph_root,
builder: self,
};
intravisit::walk_crate(&mut visitor, krate);
Expand All @@ -97,12 +96,12 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
/// Returns the child's corresponding name bindings.
fn add_child(&self,
name: Name,
parent: &Rc<Module>,
parent: Module<'b>,
duplicate_checking_mode: DuplicateCheckingMode,
// For printing errors
sp: Span)
-> NameBindings {
self.check_for_conflicts_between_external_crates_and_items(&**parent, name, sp);
-> NameBindings<'b> {
self.check_for_conflicts_between_external_crates_and_items(parent, name, sp);

// Add or reuse the child.
let child = parent.children.borrow().get(&name).cloned();
Expand Down Expand Up @@ -178,12 +177,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
return false;
}

fn get_parent_link(&mut self, parent: &Rc<Module>, name: Name) -> ParentLink {
ModuleParentLink(Rc::downgrade(parent), name)
}

/// Constructs the reduced graph for one item.
fn build_reduced_graph_for_item(&mut self, item: &Item, parent: &Rc<Module>) -> Rc<Module> {
fn build_reduced_graph_for_item(&mut self, item: &Item, parent: Module<'b>) -> Module<'b> {
let name = item.name;
let sp = item.span;
let is_public = item.vis == hir::Public;
Expand Down Expand Up @@ -238,7 +233,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
}

let subclass = SingleImport(binding, source_name);
self.build_import_directive(&**parent,
self.build_import_directive(parent,
module_path,
subclass,
view_path.span,
Expand Down Expand Up @@ -288,7 +283,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
(module_path.to_vec(), name, rename)
}
};
self.build_import_directive(&**parent,
self.build_import_directive(parent,
module_path,
SingleImport(rename, name),
source_item.span,
Expand All @@ -298,7 +293,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
}
}
ViewPathGlob(_) => {
self.build_import_directive(&**parent,
self.build_import_directive(parent,
module_path,
GlobImport,
view_path.span,
Expand All @@ -307,7 +302,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
shadowable);
}
}
parent.clone()
parent
}

ItemExternCrate(_) => {
Expand All @@ -319,32 +314,32 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
index: CRATE_DEF_INDEX,
};
self.external_exports.insert(def_id);
let parent_link = ModuleParentLink(Rc::downgrade(parent), name);
let parent_link = ModuleParentLink(parent, name);
let def = DefMod(def_id);
let external_module = Module::new(parent_link, Some(def), false, true);
let external_module = self.new_module(parent_link, Some(def), false, true);

debug!("(build reduced graph for item) found extern `{}`",
module_to_string(&*external_module));
self.check_for_conflicts_for_external_crate(&parent, name, sp);
self.check_for_conflicts_for_external_crate(parent, name, sp);
parent.external_module_children
.borrow_mut()
.insert(name, external_module.clone());
.insert(name, external_module);
self.build_reduced_graph_for_external_crate(&external_module);
}
parent.clone()
parent
}

ItemMod(..) => {
let name_bindings = self.add_child(name, parent, ForbidDuplicateTypes, sp);

let parent_link = self.get_parent_link(parent, name);
let parent_link = ModuleParentLink(parent, name);
let def = DefMod(self.ast_map.local_def_id(item.id));
let module = Module::new(parent_link, Some(def), false, is_public);
let module = self.new_module(parent_link, Some(def), false, is_public);
name_bindings.define_module(module.clone(), sp);
module
}

ItemForeignMod(..) => parent.clone(),
ItemForeignMod(..) => parent,

// These items live in the value namespace.
ItemStatic(_, m, _) => {
Expand All @@ -354,19 +349,19 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
name_bindings.define_value(DefStatic(self.ast_map.local_def_id(item.id), mutbl),
sp,
modifiers);
parent.clone()
parent
}
ItemConst(_, _) => {
self.add_child(name, parent, ForbidDuplicateValues, sp)
.define_value(DefConst(self.ast_map.local_def_id(item.id)), sp, modifiers);
parent.clone()
parent
}
ItemFn(_, _, _, _, _, _) => {
let name_bindings = self.add_child(name, parent, ForbidDuplicateValues, sp);

let def = DefFn(self.ast_map.local_def_id(item.id), false);
name_bindings.define_value(def, sp, modifiers);
parent.clone()
parent
}

// These items live in the type namespace.
Expand All @@ -376,11 +371,11 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
ForbidDuplicateTypes,
sp);

let parent_link = self.get_parent_link(parent, name);
let parent_link = ModuleParentLink(parent, name);
let def = DefTy(self.ast_map.local_def_id(item.id), false);
let module = Module::new(parent_link, Some(def), false, is_public);
let module = self.new_module(parent_link, Some(def), false, is_public);
name_bindings.define_module(module, sp);
parent.clone()
parent
}

ItemEnum(ref enum_definition, _) => {
Expand All @@ -389,9 +384,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
ForbidDuplicateTypes,
sp);

let parent_link = self.get_parent_link(parent, name);
let parent_link = ModuleParentLink(parent, name);
let def = DefTy(self.ast_map.local_def_id(item.id), true);
let module = Module::new(parent_link, Some(def), false, is_public);
let module = self.new_module(parent_link, Some(def), false, is_public);
name_bindings.define_module(module.clone(), sp);

let variant_modifiers = if is_public {
Expand All @@ -404,7 +399,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
self.build_reduced_graph_for_variant(variant, item_def_id,
&module, variant_modifiers);
}
parent.clone()
parent
}

// These items live in both the type and value namespaces.
Expand Down Expand Up @@ -444,11 +439,11 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
let item_def_id = self.ast_map.local_def_id(item.id);
self.structs.insert(item_def_id, named_fields);

parent.clone()
parent
}

ItemDefaultImpl(_, _) |
ItemImpl(..) => parent.clone(),
ItemImpl(..) => parent,

ItemTrait(_, _, _, ref items) => {
let name_bindings = self.add_child(name,
Expand All @@ -459,9 +454,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
let def_id = self.ast_map.local_def_id(item.id);

// Add all the items within to a new module.
let parent_link = self.get_parent_link(parent, name);
let parent_link = ModuleParentLink(parent, name);
let def = DefTrait(def_id);
let module_parent = Module::new(parent_link, Some(def), false, is_public);
let module_parent = self.new_module(parent_link, Some(def), false, is_public);
name_bindings.define_module(module_parent.clone(), sp);

// Add the names of all the items to the trait info.
Expand Down Expand Up @@ -494,7 +489,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
self.trait_item_map.insert((trait_item.name, def_id), trait_item_def_id);
}

parent.clone()
parent
}
}
}
Expand All @@ -504,7 +499,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
fn build_reduced_graph_for_variant(&mut self,
variant: &Variant,
item_id: DefId,
parent: &Rc<Module>,
parent: Module<'b>,
variant_modifiers: DefModifiers) {
let name = variant.node.name;
let is_exported = if variant.node.data.is_struct() {
Expand Down Expand Up @@ -534,7 +529,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
/// Constructs the reduced graph for one foreign item.
fn build_reduced_graph_for_foreign_item(&mut self,
foreign_item: &ForeignItem,
parent: &Rc<Module>) {
parent: Module<'b>) {
let name = foreign_item.name;
let is_public = foreign_item.vis == hir::Public;
let modifiers = if is_public {
Expand All @@ -555,30 +550,30 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
name_bindings.define_value(def, foreign_item.span, modifiers);
}

fn build_reduced_graph_for_block(&mut self, block: &Block, parent: &Rc<Module>) -> Rc<Module> {
fn build_reduced_graph_for_block(&mut self, block: &Block, parent: Module<'b>) -> Module<'b> {
if self.block_needs_anonymous_module(block) {
let block_id = block.id;

debug!("(building reduced graph for block) creating a new anonymous module for block \
{}",
block_id);

let parent_link = BlockParentLink(Rc::downgrade(parent), block_id);
let new_module = Module::new(parent_link, None, false, false);
parent.anonymous_children.borrow_mut().insert(block_id, new_module.clone());
let parent_link = BlockParentLink(parent, block_id);
let new_module = self.new_module(parent_link, None, false, false);
parent.anonymous_children.borrow_mut().insert(block_id, new_module);
new_module
} else {
parent.clone()
parent
}
}

fn handle_external_def(&mut self,
def: Def,
vis: Visibility,
child_name_bindings: &NameBindings,
child_name_bindings: &NameBindings<'b>,
final_ident: &str,
name: Name,
new_parent: &Rc<Module>) {
new_parent: Module<'b>) {
debug!("(building reduced graph for external crate) building external def {}, priv {:?}",
final_ident,
vis);
Expand Down Expand Up @@ -609,8 +604,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
debug!("(building reduced graph for external crate) building module {} {}",
final_ident,
is_public);
let parent_link = self.get_parent_link(new_parent, name);
let module = Module::new(parent_link, Some(def), true, is_public);
let parent_link = ModuleParentLink(new_parent, name);
let module = self.new_module(parent_link, Some(def), true, is_public);
child_name_bindings.define_module(module, DUMMY_SP);
}
}
Expand Down Expand Up @@ -681,8 +676,8 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
}

// Define a module if necessary.
let parent_link = self.get_parent_link(new_parent, name);
let module = Module::new(parent_link, Some(def), true, is_public);
let parent_link = ModuleParentLink(new_parent, name);
let module = self.new_module(parent_link, Some(def), true, is_public);
child_name_bindings.define_module(module, DUMMY_SP);
}
DefTy(..) | DefAssociatedTy(..) => {
Expand Down Expand Up @@ -728,7 +723,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {

/// Builds the reduced graph for a single item in an external crate.
fn build_reduced_graph_for_external_crate_def(&mut self,
root: &Rc<Module>,
root: Module<'b>,
xcdef: ChildItem) {
match xcdef.def {
DlDef(def) => {
Expand Down Expand Up @@ -766,9 +761,9 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {
}

/// Builds the reduced graph rooted at the given external module.
fn populate_external_module(&mut self, module: &Rc<Module>) {
fn populate_external_module(&mut self, module: Module<'b>) {
debug!("(populating external module) attempting to populate {}",
module_to_string(&**module));
module_to_string(module));

let def_id = match module.def_id() {
None => {
Expand All @@ -788,7 +783,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {

/// Ensures that the reduced graph rooted at the given external module
/// is built, building it if it is not.
fn populate_module_if_necessary(&mut self, module: &Rc<Module>) {
fn populate_module_if_necessary(&mut self, module: Module<'b>) {
if !module.populated.get() {
self.populate_external_module(module)
}
Expand All @@ -797,7 +792,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {

/// Builds the reduced graph rooted at the 'use' directive for an external
/// crate.
fn build_reduced_graph_for_external_crate(&mut self, root: &Rc<Module>) {
fn build_reduced_graph_for_external_crate(&mut self, root: Module<'b>) {
let root_cnum = root.def_id().unwrap().krate;
for child in self.session.cstore.crate_top_level_items(root_cnum) {
self.build_reduced_graph_for_external_crate_def(root, child);
Expand All @@ -806,7 +801,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {

/// Creates and adds an import directive to the given module.
fn build_import_directive(&mut self,
module_: &Module,
module_: Module<'b>,
module_path: Vec<Name>,
subclass: ImportDirectiveSubclass,
span: Span,
Expand Down Expand Up @@ -866,7 +861,7 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> {

struct BuildReducedGraphVisitor<'a, 'b: 'a, 'tcx: 'b> {
builder: GraphBuilder<'a, 'b, 'tcx>,
parent: Rc<Module>,
parent: Module<'b>,
}

impl<'a, 'b, 'v, 'tcx> Visitor<'v> for BuildReducedGraphVisitor<'a, 'b, 'tcx> {
Expand Down Expand Up @@ -897,6 +892,7 @@ pub fn build_reduced_graph(resolver: &mut Resolver, krate: &hir::Crate) {
GraphBuilder { resolver: resolver }.build_reduced_graph(krate);
}

pub fn populate_module_if_necessary(resolver: &mut Resolver, module: &Rc<Module>) {
pub fn populate_module_if_necessary<'a, 'tcx>(resolver: &mut Resolver<'a, 'tcx>,
module: Module<'a>) {
GraphBuilder { resolver: resolver }.populate_module_if_necessary(module);
}
Loading

0 comments on commit 1246f43

Please sign in to comment.