Skip to content

Commit

Permalink
Remove an Rc optimization that's no longer necessary
Browse files Browse the repository at this point in the history
This optimization ended up not being correct with the recent switch to being
more recursive, and after some profiling it looks like this optimization for
memory usage isn't even needed any more. This commit removes the `Rc` sharing,
fixing #1841 in the process.

Closes #1841
  • Loading branch information
alexcrichton committed Jul 27, 2015
1 parent 76ef150 commit 88f5bb5
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 8 deletions.
15 changes: 7 additions & 8 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@
//! dependency graph (one of the largest known ones), so hopefully it'll work
//! for a bit longer as well!

use std::cell::RefCell;
use std::collections::HashSet;
use std::collections::hash_map::HashMap;
use std::fmt;
Expand Down Expand Up @@ -232,7 +231,7 @@ impl fmt::Debug for Resolve {
struct Context {
activations: HashMap<(String, SourceId), Vec<Rc<Summary>>>,
resolve: Resolve,
visited: Rc<RefCell<HashSet<PackageId>>>,
visited: HashSet<PackageId>,
}

/// Builds the list of all packages required to build the first argument.
Expand All @@ -244,7 +243,7 @@ pub fn resolve(summary: &Summary, method: Method,
let cx = Box::new(Context {
resolve: Resolve::new(summary.package_id().clone()),
activations: HashMap::new(),
visited: Rc::new(RefCell::new(HashSet::new())),
visited: HashSet::new(),
});
let _p = profile::start(format!("resolving: {}", summary.package_id()));
match try!(activate(cx, registry, &summary, method, &mut |cx, _| Ok(Ok(cx)))) {
Expand All @@ -271,14 +270,14 @@ fn activate(mut cx: Box<Context>,
// Dependency graphs are required to be a DAG, so we keep a set of
// packages we're visiting and bail if we hit a dupe.
let id = parent.package_id();
if !cx.visited.borrow_mut().insert(id.clone()) {
if !cx.visited.insert(id.clone()) {
return Err(human(format!("cyclic package dependency: package `{}` \
depends on itself", id)))
}

// If we're already activated, then that was easy!
if cx.flag_activated(parent, &method) {
cx.visited.borrow_mut().remove(id);
cx.visited.remove(id);
return finished(cx, registry)
}
debug!("activating {}", parent.package_id());
Expand All @@ -292,8 +291,8 @@ fn activate(mut cx: Box<Context>,
};

activate_deps(cx, registry, parent, platform, deps.iter(), 0,
&mut |cx, registry| {
cx.visited.borrow_mut().remove(parent.package_id());
&mut |mut cx, registry| {
cx.visited.remove(id);
finished(cx, registry)
})
}
Expand Down Expand Up @@ -373,7 +372,7 @@ fn activate_deps<'a>(cx: Box<Context>,
// If we hit an intransitive dependency then clear out the visitation
// list as we can't induce a cycle through transitive dependencies.
if !dep.is_transitive() {
my_cx.visited.borrow_mut().clear();
my_cx.visited.clear();
}
let my_cx = try!(activate(my_cx, registry, candidate, method,
&mut |cx, registry| {
Expand Down
34 changes: 34 additions & 0 deletions tests/test_cargo_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -771,3 +771,37 @@ test!(update_transitive_dependency {
{compiling} foo v0.5.0 ([..])
", downloading = DOWNLOADING, compiling = COMPILING)));
});

test!(update_backtracking_ok {
let p = project("foo")
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.5.0"
authors = []
[dependencies]
webdriver = "0.1"
"#)
.file("src/main.rs", "fn main() {}");
p.build();

r::mock_pkg("webdriver", "0.1.0", &[("hyper", "0.6", "normal")]);
r::mock_pkg("hyper", "0.6.5", &[("openssl", "0.1", "normal"),
("cookie", "0.1", "normal")]);
r::mock_pkg("cookie", "0.1.0", &[("openssl", "0.1", "normal")]);
r::mock_pkg("openssl", "0.1.0", &[]);

assert_that(p.cargo("generate-lockfile"),
execs().with_status(0));

r::mock_pkg("openssl", "0.1.1", &[]);
r::mock_pkg("hyper", "0.6.6", &[("openssl", "0.1.1", "normal"),
("cookie", "0.1.0", "normal")]);

assert_that(p.cargo("update").arg("-p").arg("hyper"),
execs().with_status(0)
.with_stdout(&format!("\
{updating} registry `[..]`
", updating = UPDATING)));
});

0 comments on commit 88f5bb5

Please sign in to comment.