Skip to content
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

[nll] optimize redundant reborrows #53176

Closed
nikomatsakis opened this Issue Aug 7, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Aug 7, 2018

When you have a borrow like this:

let x = &*y

where y: &T, then there is really no reason for the borrow checker to "record" the borrow or figure out its scope. The reason is that there is no action that one can do with y that would invalidate x. (Of course we still want to record the outlives relationship that forces y's lifetime to outlive x's lifetime.)

The old AST borrow checker did this same optimization. It ought to have a huge effect on html5ever: my profile measurements suggest that basically all of its time is spent with these very large borrow sets.

@nikomatsakis nikomatsakis added this to the Rust 2018 RC milestone Aug 7, 2018

@nikomatsakis nikomatsakis changed the title [nll] avoid creating borrows for reborrows of shared references [nll] optimize redundant reborrows Aug 7, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Aug 7, 2018

Let me leave a few notes here. I'm not 100% sure on what changes are needed, but this is the general vicinity.

We compute the "set of all borrows" into this BorrowSet data structure:

crate struct BorrowSet<'tcx> {

In that data structure, we currently walk over the MIR and identify assignments of borrows (e.g., X = &<place>). For each of those, we allocate a borrow index... well, almost each of them. For some borrows, we don't bother -- in particular, borrows of "unsafe places":

if borrowed_place.is_unsafe_place(self.tcx, self.mir) {
return;
}

An 'unsafe place' is something like the deref of a raw pointer:

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/place_ext.rs#L27

I think we basically want to rename is_unsafe_place to something like ignore_borrow_of, and update the command like so:

/// Returns true if we can safely ignore borrows of this place.
/// This is true whenever there is no action that the user can do
/// to the place `self` that would invalidate the borrow. This is true
/// for borrows of raw pointer dereferents as well as shared references.
fn ignore_borrow_of(&self) -> bool {
    ...
}

then we will add [TyRef](https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/enum.TypeVariants.html#variant.TyRef to this match here:

match ty.sty {
ty::TyRawPtr(..) => true,
_ => proj.base.is_unsafe_place(tcx, mir),
}

Specifically, TyRef when the Mutability is MutImmutable.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Aug 7, 2018

So, I couldnt' help myself. I had a few minutes and that changed seemed so easy that I gave it a spin.

bors added a commit that referenced this issue Aug 7, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=<try>

[WIP] optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.

@nikomatsakis nikomatsakis self-assigned this Aug 8, 2018

bors added a commit that referenced this issue Aug 10, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=pnkfelix

optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.

bors added a commit that referenced this issue Aug 10, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=pnkfelix

optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.

bors added a commit that referenced this issue Aug 10, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=pnkfelix

optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.

bors added a commit that referenced this issue Aug 10, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=pnkfelix

optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.

bors added a commit that referenced this issue Aug 10, 2018

Auto merge of #53177 - nikomatsakis:nll-redundant-borrows-and-escapin…
…g-values, r=pnkfelix

optimize redundant borrows and escaping paths in NLL

This builds on #53168 and adds a commit that addresses #53176 -- or at least I think it does. I marked this as WIP because I want to see the test results (and measure the performance). I also want to double check we're not adding in any unsoundness here.
@lqd

This comment has been minimized.

Copy link
Contributor

lqd commented Aug 16, 2018

This was completed in #53177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.