Make TransitiveRelation thread safe. Avoid locking by using get_mut in some cases

[Zoxc]

r? @nikomatsakis

[nikomatsakis]

Hmm. This is a case where I suspect we could do better than the current design. I think I was just kinda' lazy before.

[nikomatsakis]

I'll take a look.

[Zoxc]

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?

[nikomatsakis]

@Zoxc no, I think that field goes away -- but I think the type is still used

[nikomatsakis]

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 is None. 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]

@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.

[nikomatsakis]

@Zoxc

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 =)

[Zoxc]

@nikomatsakis It is on this list.

[nikomatsakis]

@Zoxc oh, great! I just made https://github.com/rust-lang/rust/issues/48685...maybe we can just link to that list from there? Or close one or the other.

[nikomatsakis]

@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...

[nikomatsakis]

@bors r+

[bors]

📌 Commit 89e55d1 has been approved by nikomatsakis

[bors]

⌛ Testing commit 89e55d1 with merge 9ff5cb5...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 9ff5cb5 to master...