remove use of upgradable reads from derived queries #75
Conversation
|
I'm now thinking that this PR is not necessary. But I'm trying to write out a comment clearly explaining what went wrong and why I think we're ok and dang is it hard. =) The TL;DR of why I think we're ok now is once we acquire this particular upgradable-read, we will always release it without blocking on any other locks (apart from the mutex guarding the dependency graph, but that is always a "leaf lock" so we should be ok). |
|
I feel maybe we should stick to read/write access for the time being, just to make debugging deadlocks, should they reappear, easier. That is, I'd like to avoid thinking "oh, maybe its that upgrade lock again" when debugging. We probably can put upgradable reads in back when parking_lot is fixed. |
I think the main problem here is that parking_lot's behavior does not match the docs, so it's difficult to reason about what are you trying to prove in the first place. |
|
@nikomatsakis see #99 for a deadlock which happens on those |
nikomatsakis
force-pushed
the
issue-70
branch
from
5ca6e11
to
2e4ff9a
Compare
|
I'd like to understand this deadlock, but I rebased. |
I've mapped where each thread is blocked over here: This doesn't look like the "upgradable reads block reads" situation we've got before, because only writes and upgradable reads are involved, no plain reads. The next step would probably be to write a parking-lot stress test, like the one for reads. I sort of expect an "interesting" behavior of upgradable read here :) |
|
@nikomatsakis see Amanieu/parking_lot#101 (comment) (a new example, mirroring what this PR is doing). Looks like it's a pl bug. |
|
Fascinating. OK, merging and publishing! |
- Panic safety improvements (salsa-rs#81) - We build on stable now (salsa-rs#94) - Removed use of dynamic dispatch for constructing query descriptors (salsa-rs#95) - Technically a breaking change, though unlikely to affect clients - Removed use of upgradable reads to avoid Amanieu/parking-lot#101 (salsa-rs#75) - Upgraded parking lot (salsa-rs#100) - Improved Debug output (salsa-rs#98) - Snapshot implements Debug (salsa-rs#85)
|
This bug is fixed in parking lot 0.7.1 so in theory we can perhaps revert this now? Though, I'd rather avoid using upgradable reads: even if they are bug free now, I'll still will be blaiming/testing them when facing with further salsa bugs :-) And in general, it might be a good idea to improve locking infra once with have benchmarks to quantify improvements (FWIW, salsa is fast enough for everything I've thrown at it in rust-analyzer so far) :-) |
No description provided.