Don't hold the predecessor cache lock longer than necessary #71392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
ecstatic-morse
changed the title
|
☔ The latest upstream changes (presumably #71323) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
approved these changes
Apr 22, 2020
nikomatsakis
added
S-waiting-on-author
|
@bors delegate=ecstatic-morse |
|
✌️ @ecstatic-morse can now approve this pull request |
There is no `Arc::map` equivalent to `LockGuard::map`
...instead of a `LockGuard` which means the lock is held for longer than necessary.
ecstatic-morse
force-pushed
the
body-predecessor-cache-arc
branch
from
00ac196
to
59c7460
Compare
|
Rebased and restricted visibility of the @bors r=nikomatsakis |
|
📌 Commit 34dfbc3 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Apr 24, 2020
…he-arc, r=nikomatsakis Don't hold the predecessor cache lock longer than necessary rust-lang#71044 returns a `LockGuard` with the predecessor cache to callers of `Body::predecessors`. As a result, the lock around the predecessor cache could be held for an arbitrarily long time. This PR uses reference counting for ownership of the predecessor cache, meaning the lock is only ever held within `PredecessorCache::compute`. Checking this API for potential sources of deadlock is much easier now, since we no longer have to consider its consumers, only its internals. This required removing `predecessors_for`, since there is no equivalent to `LockGuard::map` for `Arc` and `Rc`. I believe this could be emulated with `owning_ref::{Arc,Rc}Ref`, but I don't think it's necessary. Also, we continue to return an opaque type from `Body::predecessors` with the lifetime of the `Body`, not `'static`. This depends on rust-lang#71044. Only the last two commits are new. r? @nikomatsakis
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Apr 25, 2020
…he-arc, r=nikomatsakis Don't hold the predecessor cache lock longer than necessary rust-lang#71044 returns a `LockGuard` with the predecessor cache to callers of `Body::predecessors`. As a result, the lock around the predecessor cache could be held for an arbitrarily long time. This PR uses reference counting for ownership of the predecessor cache, meaning the lock is only ever held within `PredecessorCache::compute`. Checking this API for potential sources of deadlock is much easier now, since we no longer have to consider its consumers, only its internals. This required removing `predecessors_for`, since there is no equivalent to `LockGuard::map` for `Arc` and `Rc`. I believe this could be emulated with `owning_ref::{Arc,Rc}Ref`, but I don't think it's necessary. Also, we continue to return an opaque type from `Body::predecessors` with the lifetime of the `Body`, not `'static`. This depends on rust-lang#71044. Only the last two commits are new. r? @nikomatsakis
This was referenced Apr 25, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 26, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#70043 (Add all remaining `DefKind`s.) - rust-lang#71140 ([breaking change] Disallow statics initializing themselves) - rust-lang#71392 (Don't hold the predecessor cache lock longer than necessary) - rust-lang#71541 (Add regression test for rust-lang#26376) - rust-lang#71554 (Replace thread_local with generator resume arguments in box_region.) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#71044 returns a
LockGuardwith the predecessor cache to callers ofBody::predecessors. As a result, the lock around the predecessor cache could be held for an arbitrarily long time. This PR uses reference counting for ownership of the predecessor cache, meaning the lock is only ever held withinPredecessorCache::compute. Checking this API for potential sources of deadlock is much easier now, since we no longer have to consider its consumers, only its internals.This required removing
predecessors_for, since there is no equivalent toLockGuard::mapforArcandRc. I believe this could be emulated withowning_ref::{Arc,Rc}Ref, but I don't think it's necessary. Also, we continue to return an opaque type fromBody::predecessorswith the lifetime of theBody, not'static.This depends on #71044. Only the last two commits are new.
r? @nikomatsakis