-
Notifications
You must be signed in to change notification settings - Fork 155
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
re-enable parallel tests #527
Conversation
We are using "peek fields" methods instead; I like that approach better.
We are going to want to take databases at public entry points.
We can use this to more reliably sort.
We used to sort just by the ingredient index, but since those are now added dynamically, that can be fairly unstable in some of the tests. We now sort by the "debug name" of the ingredient first, which is more reliably stable.
I realized there weren't any!
✅ Deploy Preview for salsa-rs canceled.
|
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.
I like the idea of a handle. It also removes the need for implementing a snapshot function on any other field stored on Db
(which normally involves wrapping that field in an Arc
).
The only thing that I'm slightly concerned about is the cost of using thread locals for every Salsa operation.
src/handle.rs
Outdated
@@ -49,6 +49,12 @@ impl<Db: HasStorage> Handle<Db> { | |||
let storage = self.db.storage(); | |||
storage.runtime().set_cancellation_flag(); | |||
|
|||
self.db.salsa_event(Event { |
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: We could probably use an AtomicUsize
for clones
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.
Yes, we could, we do need a mutex for signalling. This is simpler.
interesting, I don't see this... |
Sigh, I always make this mistake.
This branch re-enables the parallel tests. It does not yet add the nicer parallel APIs imagined for Salsa 3.0.
Changes:
salsa::Handle
, which can simply be cloned.get_mut
method on theHandle
is called, any outstanding clones will be cancelled (I added a test for this).