From d01d6ed511fc46b6bd8d7bc2de5e5896da008ed0 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 19 Dec 2018 02:40:02 +0300 Subject: [PATCH 1/2] Make GC API more orthogonal and flexible Now, the effect of GC is a "product" of three parameters: * what values are affected (everything/everything except used) * are we removing values * are we removing deps SweepStrategy::default is now a no-op GC. --- src/derived.rs | 5 ++++- src/lib.rs | 30 +++++++++++++++++++++++++++++- tests/gc/derived_tests.rs | 10 +++++----- tests/gc/discard_values.rs | 10 +++++++--- tests/gc/shallow_constant_tests.rs | 8 ++++---- tests/parallel/stress.rs | 2 +- 6 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index dcebf4f2..9e0e8c4e 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -971,12 +971,15 @@ where // revision, since we are holding the write lock // when we read `revision_now`. assert!(memo.verified_at <= revision_now); + if strategy.keep_current_revision && memo.verified_at == revision_now { + return true; + } if !strategy.keep_values { memo.value = None; } - memo.verified_at == revision_now + strategy.keep_deps } } }); diff --git a/src/lib.rs b/src/lib.rs index 270a689b..78a2cfbf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -211,9 +211,16 @@ impl fmt::Debug for EventKind { #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct SweepStrategy { keep_values: bool, + keep_deps: bool, + keep_current_revision: bool, } impl SweepStrategy { + /// Fully discards keys not used in the current revision + pub fn discard_old() -> SweepStrategy { + SweepStrategy::default().discard_deps() + } + /// Causes us to discard memoized *values* but keep the /// *dependencies*. This means you will have to recompute the /// results from any queries you execute but does permit you to @@ -224,11 +231,32 @@ impl SweepStrategy { ..self } } + /// Causes us to discard both memoized *values* and *dependencies*. + pub fn discard_deps(self) -> SweepStrategy { + SweepStrategy { + keep_deps: false, + ..self + } + } + + /// Causes us to collect keys from all revisions. + /// + /// By default, only keys not used in the current revision are collected. + pub fn discard_all_revisions(self) -> SweepStrategy { + SweepStrategy { + keep_current_revision: false, + ..self + } + } } impl Default for SweepStrategy { fn default() -> Self { - SweepStrategy { keep_values: true } + SweepStrategy { + keep_values: true, + keep_deps: true, + keep_current_revision: true, + } } } diff --git a/tests/gc/derived_tests.rs b/tests/gc/derived_tests.rs index 87f19162..e68a76e2 100644 --- a/tests/gc/derived_tests.rs +++ b/tests/gc/derived_tests.rs @@ -25,7 +25,7 @@ fn compute_one() { // Memoized, but will compute fibonacci(5) again db.compute(5); - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, @@ -64,7 +64,7 @@ fn compute_switch() { MaxQuery => (), } - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); // Now we just have `Triangular` and not `Fibonacci` assert_keys! { @@ -80,7 +80,7 @@ fn compute_switch() { // Now run `compute` *again* in next revision. db.salsa_runtime().next_revision(); assert_eq!(db.compute(5), 15); - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); // We keep triangular, but just the outermost one. assert_keys! { @@ -109,7 +109,7 @@ fn compute_all() { db.compute_all(); db.salsa_runtime().next_revision(); db.compute_all(); - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, @@ -137,7 +137,7 @@ fn compute_all() { MaxQuery => (()), } - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); // We no longer used `Compute(5)` and `Triangular(5)`; note that // `UseTriangularQuery(5)` is not collected, as it is an input. diff --git a/tests/gc/discard_values.rs b/tests/gc/discard_values.rs index 0c10c1da..659dc939 100644 --- a/tests/gc/discard_values.rs +++ b/tests/gc/discard_values.rs @@ -19,7 +19,7 @@ fn sweep_default() { // fibonacci is a constant, so it will not be invalidated, // hence we keep 3 and 5 but remove the rest. - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, FibonacciQuery => (3, 5), @@ -31,8 +31,12 @@ fn sweep_default() { db.assert_log(&[]); // Same but we discard values this time. - db.sweep_all(SweepStrategy::default().discard_values()); - db.sweep_all(SweepStrategy::default()); + db.sweep_all( + SweepStrategy::default() + .discard_values() + .discard_all_revisions(), + ); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, FibonacciQuery => (3, 5), diff --git a/tests/gc/shallow_constant_tests.rs b/tests/gc/shallow_constant_tests.rs index 423ce010..25c0abc4 100644 --- a/tests/gc/shallow_constant_tests.rs +++ b/tests/gc/shallow_constant_tests.rs @@ -18,7 +18,7 @@ fn one_rev() { // Everything was used in this revision, so // nothing gets collected. - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_eq!(k.len(), 6); } @@ -35,7 +35,7 @@ fn two_rev_nothing() { // Nothing was used in this revision, so // everything gets collected. - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); let k: Vec<_> = db.query(FibonacciQuery).entries(); assert_eq!(k.len(), 0); @@ -56,7 +56,7 @@ fn two_rev_one_use() { // fibonacci is a constant, so it will not be invalidated, // hence we keep `fibonacci(5)` but remove 0..=4. - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, @@ -80,7 +80,7 @@ fn two_rev_two_uses() { // fibonacci is a constant, so it will not be invalidated, // hence we keep 3 and 5 but remove the rest. - db.sweep_all(SweepStrategy::default()); + db.sweep_all(SweepStrategy::discard_old()); assert_keys! { db, diff --git a/tests/parallel/stress.rs b/tests/parallel/stress.rs index 152cfbfe..e9ce67fd 100644 --- a/tests/parallel/stress.rs +++ b/tests/parallel/stress.rs @@ -114,7 +114,7 @@ impl rand::distributions::Distribution for rand::distributions::Standard let key = rng.gen::() % 10; return ReadOp::Get(query, key); } - let mut strategy = SweepStrategy::default(); + let mut strategy = SweepStrategy::discard_old(); if rng.gen_bool(0.5) { strategy = strategy.discard_values(); } From 9387fd2f4d31210daa8805f0f0b8e4d22e73c3ea Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 27 Jan 2019 17:14:57 +0300 Subject: [PATCH 2/2] more orthogonal naming --- src/derived.rs | 24 ++++---- src/lib.rs | 94 ++++++++++++++++++++---------- tests/gc/derived_tests.rs | 10 ++-- tests/gc/discard_values.rs | 6 +- tests/gc/shallow_constant_tests.rs | 8 +-- tests/parallel/stress.rs | 2 +- 6 files changed, 90 insertions(+), 54 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index 9e0e8c4e..a5f2ada4 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -11,7 +11,7 @@ use crate::runtime::Revision; use crate::runtime::Runtime; use crate::runtime::RuntimeId; use crate::runtime::StampedValue; -use crate::{Database, Event, EventKind, SweepStrategy}; +use crate::{Database, DiscardIf, DiscardWhat, Event, EventKind, SweepStrategy}; use log::{debug, info}; use parking_lot::Mutex; use parking_lot::RwLock; @@ -953,7 +953,8 @@ where true } - // Otherwise, keep only if it was used in this revision. + // Otherwise, drop only value or the whole memo accoring to the + // strategy. QueryState::Memoized(memo) => { debug!( "sweep({:?}({:?})): last verified at {:?}, current revision {:?}", @@ -971,15 +972,18 @@ where // revision, since we are holding the write lock // when we read `revision_now`. assert!(memo.verified_at <= revision_now); - if strategy.keep_current_revision && memo.verified_at == revision_now { - return true; + match strategy.discard_if { + DiscardIf::Never => true, + DiscardIf::Outdated if memo.verified_at == revision_now => true, + DiscardIf::Outdated | DiscardIf::Always => match strategy.discard_what { + DiscardWhat::Nothing => true, + DiscardWhat::Values => { + memo.value = None; + true + } + DiscardWhat::Everything => false, + }, } - - if !strategy.keep_values { - memo.value = None; - } - - strategy.keep_deps } } }); diff --git a/src/lib.rs b/src/lib.rs index 78a2cfbf..60a81e82 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -205,57 +205,89 @@ impl fmt::Debug for EventKind { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum DiscardIf { + Never, + Outdated, + Always, +} + +impl Default for DiscardIf { + fn default() -> DiscardIf { + DiscardIf::Never + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +enum DiscardWhat { + Nothing, + Values, + Everything, +} + +impl Default for DiscardWhat { + fn default() -> DiscardWhat { + DiscardWhat::Nothing + } +} + /// The sweep strategy controls what data we will keep/discard when we -/// do a GC-sweep. The default (`SweepStrategy::default`) is to keep -/// all memoized values used in the current revision. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] +/// do a GC-sweep. The default (`SweepStrategy::default`) is a no-op, +/// use `SweepStrategy::discard_outdated` constructor or `discard_*` +/// and `sweep_*` builder functions to construct useful strategies. +#[derive(Debug, Copy, Clone, Default, PartialEq, Eq)] pub struct SweepStrategy { - keep_values: bool, - keep_deps: bool, - keep_current_revision: bool, + discard_if: DiscardIf, + discard_what: DiscardWhat, } impl SweepStrategy { - /// Fully discards keys not used in the current revision - pub fn discard_old() -> SweepStrategy { - SweepStrategy::default().discard_deps() + /// Convenience function that discards all data not used thus far in the + /// current revision. + /// + /// Equivalent to `SweepStrategy::default().discard_everything()`. + pub fn discard_outdated() -> SweepStrategy { + SweepStrategy::default() + .discard_everything() + .sweep_outdated() } - /// Causes us to discard memoized *values* but keep the - /// *dependencies*. This means you will have to recompute the - /// results from any queries you execute but does permit you to - /// quickly determine if a value is still up to date. + /// Collects query values. + /// + /// Query dependencies are left in the database, which allows to quickly + /// determine if the query is up to date, and avoid recomputing + /// dependencies. pub fn discard_values(self) -> SweepStrategy { SweepStrategy { - keep_values: false, - ..self - } - } - /// Causes us to discard both memoized *values* and *dependencies*. - pub fn discard_deps(self) -> SweepStrategy { - SweepStrategy { - keep_deps: false, + discard_what: self.discard_what.max(DiscardWhat::Values), ..self } } - /// Causes us to collect keys from all revisions. + /// Collects both values and information about dependencies. /// - /// By default, only keys not used in the current revision are collected. - pub fn discard_all_revisions(self) -> SweepStrategy { + /// Dependant queries will be recomputed even if all inputs to this query + /// stay the same. + pub fn discard_everything(self) -> SweepStrategy { SweepStrategy { - keep_current_revision: false, + discard_what: self.discard_what.max(DiscardWhat::Everything), ..self } } -} -impl Default for SweepStrategy { - fn default() -> Self { + /// Process all keys, not verefied at the current revision. + pub fn sweep_outdated(self) -> SweepStrategy { SweepStrategy { - keep_values: true, - keep_deps: true, - keep_current_revision: true, + discard_if: self.discard_if.max(DiscardIf::Outdated), + ..self + } + } + + /// Process all keys. + pub fn sweep_all_revisions(self) -> SweepStrategy { + SweepStrategy { + discard_if: self.discard_if.max(DiscardIf::Always), + ..self } } } diff --git a/tests/gc/derived_tests.rs b/tests/gc/derived_tests.rs index e68a76e2..ddaa7ee9 100644 --- a/tests/gc/derived_tests.rs +++ b/tests/gc/derived_tests.rs @@ -25,7 +25,7 @@ fn compute_one() { // Memoized, but will compute fibonacci(5) again db.compute(5); - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, @@ -64,7 +64,7 @@ fn compute_switch() { MaxQuery => (), } - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); // Now we just have `Triangular` and not `Fibonacci` assert_keys! { @@ -80,7 +80,7 @@ fn compute_switch() { // Now run `compute` *again* in next revision. db.salsa_runtime().next_revision(); assert_eq!(db.compute(5), 15); - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); // We keep triangular, but just the outermost one. assert_keys! { @@ -109,7 +109,7 @@ fn compute_all() { db.compute_all(); db.salsa_runtime().next_revision(); db.compute_all(); - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, @@ -137,7 +137,7 @@ fn compute_all() { MaxQuery => (()), } - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); // We no longer used `Compute(5)` and `Triangular(5)`; note that // `UseTriangularQuery(5)` is not collected, as it is an input. diff --git a/tests/gc/discard_values.rs b/tests/gc/discard_values.rs index 659dc939..2db2b60f 100644 --- a/tests/gc/discard_values.rs +++ b/tests/gc/discard_values.rs @@ -19,7 +19,7 @@ fn sweep_default() { // fibonacci is a constant, so it will not be invalidated, // hence we keep 3 and 5 but remove the rest. - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, FibonacciQuery => (3, 5), @@ -34,9 +34,9 @@ fn sweep_default() { db.sweep_all( SweepStrategy::default() .discard_values() - .discard_all_revisions(), + .sweep_all_revisions(), ); - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, FibonacciQuery => (3, 5), diff --git a/tests/gc/shallow_constant_tests.rs b/tests/gc/shallow_constant_tests.rs index 25c0abc4..a5eef000 100644 --- a/tests/gc/shallow_constant_tests.rs +++ b/tests/gc/shallow_constant_tests.rs @@ -18,7 +18,7 @@ fn one_rev() { // Everything was used in this revision, so // nothing gets collected. - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_eq!(k.len(), 6); } @@ -35,7 +35,7 @@ fn two_rev_nothing() { // Nothing was used in this revision, so // everything gets collected. - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); let k: Vec<_> = db.query(FibonacciQuery).entries(); assert_eq!(k.len(), 0); @@ -56,7 +56,7 @@ fn two_rev_one_use() { // fibonacci is a constant, so it will not be invalidated, // hence we keep `fibonacci(5)` but remove 0..=4. - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, @@ -80,7 +80,7 @@ fn two_rev_two_uses() { // fibonacci is a constant, so it will not be invalidated, // hence we keep 3 and 5 but remove the rest. - db.sweep_all(SweepStrategy::discard_old()); + db.sweep_all(SweepStrategy::discard_outdated()); assert_keys! { db, diff --git a/tests/parallel/stress.rs b/tests/parallel/stress.rs index e9ce67fd..8b702b8b 100644 --- a/tests/parallel/stress.rs +++ b/tests/parallel/stress.rs @@ -114,7 +114,7 @@ impl rand::distributions::Distribution for rand::distributions::Standard let key = rng.gen::() % 10; return ReadOp::Get(query, key); } - let mut strategy = SweepStrategy::discard_old(); + let mut strategy = SweepStrategy::discard_outdated(); if rng.gen_bool(0.5) { strategy = strategy.discard_values(); }