-
Notifications
You must be signed in to change notification settings - Fork 152
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
introduce parallel salsa #568
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
use std::ops::Deref; | ||
|
||
use rayon::iter::{FromParallelIterator, IntoParallelIterator, ParallelIterator}; | ||
|
||
use crate::Database; | ||
|
||
pub fn par_map<Db, D, E, C>( | ||
db: &Db, | ||
inputs: impl IntoParallelIterator<Item = D>, | ||
op: fn(&Db, D) -> E, | ||
) -> C | ||
where | ||
Db: Database + ?Sized, | ||
D: Send, | ||
E: Send + Sync, | ||
C: FromParallelIterator<E>, | ||
{ | ||
let parallel_db = ParallelDb::Ref(db.as_dyn_database()); | ||
|
||
inputs | ||
.into_par_iter() | ||
.map_with(parallel_db, |parallel_db, element| { | ||
let db = parallel_db.as_view::<Db>(); | ||
op(db, element) | ||
}) | ||
.collect() | ||
} | ||
|
||
/// This enum _must not_ be public or used outside of `par_map`. | ||
enum ParallelDb<'db> { | ||
Ref(&'db dyn Database), | ||
Fork(Box<dyn Database + Send>), | ||
} | ||
|
||
/// SAFETY: the contents of the database are never accessed on the thread | ||
/// where this wrapper type is created. | ||
unsafe impl Send for ParallelDb<'_> {} | ||
|
||
impl Deref for ParallelDb<'_> { | ||
type Target = dyn Database; | ||
|
||
fn deref(&self) -> &Self::Target { | ||
match self { | ||
ParallelDb::Ref(db) => *db, | ||
ParallelDb::Fork(db) => db.as_dyn_database(), | ||
} | ||
} | ||
} | ||
|
||
impl Clone for ParallelDb<'_> { | ||
fn clone(&self) -> Self { | ||
ParallelDb::Fork(self.fork_db()) | ||
} | ||
} | ||
davidbarsky marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ use crate::{ | |
/// | ||
/// The `storage` and `storage_mut` fields must both return a reference to the same | ||
/// storage field which must be owned by `self`. | ||
pub unsafe trait HasStorage: Database + Sized { | ||
pub unsafe trait HasStorage: Database + Clone + Sized { | ||
fn storage(&self) -> &Storage<Self>; | ||
fn storage_mut(&mut self) -> &mut Storage<Self>; | ||
} | ||
|
@@ -108,6 +108,10 @@ unsafe impl<T: HasStorage> ZalsaDatabase for T { | |
fn zalsa_local(&self) -> &ZalsaLocal { | ||
&self.storage().zalsa_local | ||
} | ||
|
||
fn fork_db(&self) -> Box<dyn Database> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Micha: I'm going off the designed specified in the section titled "Clone alternative". Happy to revisit this, if needed! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't readily do that because of dyn safety, but there are other ways we can structure it to expose a variation that returns a |
||
Box::new(self.clone()) | ||
} | ||
} | ||
|
||
impl<Db: Database> RefUnwindSafe for Storage<Db> {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
// test for rayon interations. | ||
|
||
use salsa::Cancelled; | ||
use salsa::Setter; | ||
|
||
use crate::setup::Knobs; | ||
use crate::setup::KnobsDatabase; | ||
|
||
#[salsa::input] | ||
struct ParallelInput { | ||
field: Vec<u32>, | ||
} | ||
|
||
#[salsa::tracked] | ||
fn tracked_fn(db: &dyn salsa::Database, input: ParallelInput) -> Vec<u32> { | ||
salsa::par_map(db, input.field(db), |_db, field| field + 1) | ||
} | ||
|
||
#[test] | ||
fn execute() { | ||
let db = salsa::DatabaseImpl::new(); | ||
|
||
let counts = (1..=10).collect::<Vec<u32>>(); | ||
let input = ParallelInput::new(&db, counts); | ||
|
||
tracked_fn(&db, input); | ||
} | ||
|
||
#[salsa::tracked] | ||
fn a1(db: &dyn KnobsDatabase, input: ParallelInput) -> Vec<u32> { | ||
db.signal(1); | ||
salsa::par_map(db, input.field(db), |db, field| { | ||
db.wait_for(2); | ||
field + 1 | ||
}) | ||
} | ||
|
||
#[salsa::tracked] | ||
fn dummy(_db: &dyn KnobsDatabase, _input: ParallelInput) -> ParallelInput { | ||
panic!("should never get here!") | ||
} | ||
|
||
// we expect this to panic, as `salsa::par_map` needs to be called from a query. | ||
#[test] | ||
#[should_panic] | ||
fn direct_calls_panic() { | ||
let db = salsa::DatabaseImpl::new(); | ||
|
||
let counts = (1..=10).collect::<Vec<u32>>(); | ||
let input = ParallelInput::new(&db, counts); | ||
let _: Vec<u32> = salsa::par_map(&db, input.field(&db), |_db, field| field + 1); | ||
} | ||
|
||
// Cancellation signalling test | ||
// | ||
// The pattern is as follows. | ||
// | ||
// Thread A Thread B | ||
// -------- -------- | ||
// a1 | ||
// | wait for stage 1 | ||
// signal stage 1 set input, triggers cancellation | ||
// wait for stage 2 (blocks) triggering cancellation sends stage 2 | ||
// | | ||
// (unblocked) | ||
// dummy | ||
// panics | ||
|
||
#[test] | ||
fn execute_cancellation() { | ||
let mut db = Knobs::default(); | ||
|
||
let counts = (1..=10).collect::<Vec<u32>>(); | ||
let input = ParallelInput::new(&db, counts); | ||
|
||
let thread_a = std::thread::spawn({ | ||
let db = db.clone(); | ||
move || a1(&db, input) | ||
}); | ||
|
||
let counts = (2..=20).collect::<Vec<u32>>(); | ||
|
||
db.signal_on_did_cancel.store(2); | ||
input.set_field(&mut db).to(counts); | ||
|
||
// Assert thread A *should* was cancelled | ||
let cancelled = thread_a | ||
.join() | ||
.unwrap_err() | ||
.downcast::<Cancelled>() | ||
.unwrap(); | ||
|
||
// and inspect the output | ||
expect_test::expect![[r#" | ||
PendingWrite | ||
"#]] | ||
.assert_debug_eq(&cancelled); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I kind of preferred the old salsa's
ParallelDatabase
trait rather than relying onClone
. It makes the contract more explicit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's certainly option, but I'm not sure
ParallelDatabase
works well with when an end-user of Salsa only has&dyn HirDatabase
and it should be safe and reasonable to use Rayon with queries that use&dyn HirDatabase
. Like I said in my other comment, I'm going off of this document, but I'd be happy to change this design! I mostly care about making the example work with any database.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I was debating about in those variations-- there might be a hybrid here. That said, I think that ALL databases will effectively be parallel databases under this plan (that is ~required to allow queries to use parallelism internally, since they are written against a
dyn Database
).I guess we could still make a
ParallelDatabase
trait and have them work against adyn ParallelDatabase
, but I'm not convinced there's a use case for "non-parallel databases".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in rust-analyzer's case, I can't really think of a situation where it wouldn't make sense to have every
Database
be aParallelDatabase
. I don't think we'd want to make everything parallel, but having the option to do so without extensive refactors is really valuable, in my view.