introduce parallel salsa #568
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
| @@ -77,11 +81,12 @@ trait Db: salsa::Database { | |||
| } | |||
|
|
|||
| #[salsa::db] | |||
| #[derive(Clone)] | |||
There was a problem hiding this comment.
Nit: I kind of preferred the old salsa's ParallelDatabase trait rather than relying on Clone. It makes the contract more explicit.
There was a problem hiding this comment.
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.
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 a dyn ParallelDatabase, but I'm not convinced there's a use case for "non-parallel databases".
There was a problem hiding this comment.
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 a ParallelDatabase. 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.
| @@ -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.
Could we return T instead of a Box<dyn Database>?
There was a problem hiding this comment.
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.
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 Self.
davidbarsky
force-pushed
the
davidbarsky/push-lqummkkxvkzn
branch
from
0668aec
to
8e4ca38
Compare
CodSpeed Performance ReportMerging #568 will not alter performanceComparing Summary
|
src/par_map.rs
Outdated
| E: Send + Sync, | ||
| C: FromParallelIterator<E>, | ||
| { | ||
| dbg!(db.zalsa().views()); |
There was a problem hiding this comment.
i've been doing a bunch of digging to figure out why this panics when used with tests/parallel/parallel_map.rs test. i initially saw that the underling Zalza::views was empty, so the test would naturally panic—there is nothing to downcast to! I've since added calls to tracked_fn in the test, and now, I'm seeing a downcast-able database:
[src/par_map.rs:16:5] db.zalsa().views() = DynDowncasts {
vec: [
DynDowncast(
"dyn parallel_map::common::LogDatabase",
),
],
This makes sense! databases are discovered upon first use, but for par_map to work, I think the list of databases to cast to need to be reflexive: that is, they need to include themselves. I'll try making this change.
There was a problem hiding this comment.
Ah, yeah, that's easy enough to fix. I was wondering if we'd encounter an issue where the correct view wasn't registered but I think we can fix that too if needed easily enough.
src/par_map.rs
Outdated
| E: Send + Sync, | ||
| C: FromParallelIterator<E>, | ||
| { | ||
| dbg!(db.zalsa().views()); |
There was a problem hiding this comment.
Ah, yeah, that's easy enough to fix. I was wondering if we'd encounter an issue where the correct view wasn't registered but I think we can fix that too if needed easily enough.
| @@ -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.
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 Self.
davidbarsky
force-pushed
the
davidbarsky/push-lqummkkxvkzn
branch
2 times, most recently
from
cbd7159
to
acd5acf
Compare
|
The Miri errors appear to be coming from |
Judging from the issue tracker that crate seems to have a couple stacked borrows violations |
|
Does miri have a way to add 'known failures'... |
|
@davidbarsky I'd be ok moving the miri tests to "warn only" for the time being and then trying to investigate the use of crossbeam-epoch more closely. |
Lemme see about doing that. I think I still need to port over a test or two over as well. |
davidbarsky
force-pushed
the
davidbarsky/push-lqummkkxvkzn
branch
from
acd5acf
to
fb7d0f3
Compare
|
Added a cancellation test. |
davidbarsky
force-pushed
the
davidbarsky/push-lqummkkxvkzn
branch
2 times, most recently
from
a56731f
to
132ce16
Compare
davidbarsky
force-pushed
the
davidbarsky/push-lqummkkxvkzn
branch
from
132ce16
to
0102ffb
Compare
|
@davidbarsky what's the status of this PR? We now have a use case where the hacky hand rolled concurrency no longer works 😆 |
Initial pass; very much not ready. Putting it up for initial feedback; I'm still figuring out how to wrap
&Dbreasonably.