Make TransitiveRelation thread safe. Avoid locking by using get_mut in some cases by Zoxc · Pull Request #48587 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Hmm. This is a case where I suspect we could do better than the current design. I think I was just kinda' lazy before.
I think this is only shared because of the free_region_map field of TypeckTables. What that still be there if we remove the old borrowck?
@Zoxc no, I think that field goes away -- but I think the type is still used
I just confirmed that it is not used from the MIR borrow check (though the type itself is used).
I think what I would prefer to do in this case is to have the cache always be up-to-date.
We could do this in two ways:
- Remove the refcell. Clear cache to
None(as today) on every mutation, but add a method to recompute the transitive cache. Panic if you try to "read" from the relation and the transitive cache isNone. It is now on us to ensure we invoke the method at the right times, but it will eagerly panic if we forget. - Just update the cache every single time. This might be a bit of a perf hit because we'll come to a full transitive closure for every change, rather than waiting till everything is available, but honestly I doubt it.
- Something like the first variant but where we have a "freeze" method that converts from an "in-progress" FRR to one that is transitiviely up to date. More complex but avoids possibility of dynamic error.
I guess between the two I lean towards the second, as it is more robust, and we can always handle it if that is some kind of perf hit.
Would you be up for that?
@Zoxc no, I think that field goes away -- but I think the type is still used
The question isn't whether or not it's used, but whether or not it's shared. That field is the reason it's shared, so if it goes away, we could just revert this patch.
I'd prefer to merge this and then later merge some other scheme backed by benchmarks if it turns out to actually be shared.
I'd prefer to merge this and then later merge some other scheme backed by benchmarks if it turns out to actually be shared.
It will not be shared in the future, that much is sure, but I think that we probably will not remove the old borrow checker "in time". Is there some central issue tracking the refactorings? I'm willing to land this change as long as long as there is a list somewhere of stuff to fix and it is on there =)
16 tasks
@Zoxc ok I added it to my list of 'pending refactorings' here #48685 -- I'll probably open up an issue with mentoring instructions at some point. In the meantime, let me review this...
📌 Commit 89e55d1 has been approved by nikomatsakis
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Manishearth added a commit to Manishearth/rust that referenced this pull request
…sakis
Make TransitiveRelation thread safe. Avoid locking by using get_mut in some cases
bors added a commit that referenced this pull request
Make TransitiveRelation thread safe. Avoid locking by using get_mut in some cases
bors added a commit to rust-lang-ci/rust that referenced this pull request
get rid of RefCell in TransitiveRelation
This is one of the jobs in Pending refactorings in rust-lang#48685. The parallel-compiler's work has been suspended for quite some time, but I think I can pick it up gradually. I think this PR should be a start.
Regarding the refactoring of TransitiveRelation, @nikomatsakis has proposed [two(three?) schemes](rust-lang#48587 (comment)). In order to satisfy both compilation efficiency and robustness, I think adding the freeze method may be the best solution, although it requires relatively more code changes.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.