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] hash borrows in scope for better performance #53159

Closed
nikomatsakis opened this issue Aug 7, 2018 · 8 comments
Closed

[nll] hash borrows in scope for better performance #53159

nikomatsakis opened this issue Aug 7, 2018 · 8 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-performant Working towards the "performance is good" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-performance Working group: Compiler Performance

Comments

@nikomatsakis
Copy link
Contributor

So I have an idea for how to improve performance in cases like html5ever where there are tons of borrows in scope at certain points. Right now, we have a pretty naive algorithm that implements over all borrows in scope:

/// Encapsulates the idea of iterating over every borrow that involves a particular path
pub(super) fn each_borrow_involving_path<'a, 'tcx, 'gcx: 'tcx, F, I, S> (
s: &mut S,
tcx: TyCtxt<'a, 'gcx, 'tcx>,
mir: &Mir<'tcx>,
_context: Context,
access_place: (ShallowOrDeep, &Place<'tcx>),
borrow_set: &BorrowSet<'tcx>,
candidates: I,
mut op: F,
) where
F: FnMut(&mut S, BorrowIndex, &BorrowData<'tcx>) -> Control,
I: Iterator<Item=BorrowIndex>

I think we would do better if we stored the "borrows in scope" organized by the "root local" of the borrow:

/// If this is a place like `x.f.g`, returns the local
/// `x`. Returns `None` if this is based in a static.
fn root_local(&self) -> Option<Local>;

In fact, I think we could do a "quick hack" to make this work. The BorrowSet already has an index of borrow indices based on their "root locals":

/// Map from local to all the borrows on that local
crate local_map: FxHashMap<mir::Local, FxHashSet<BorrowIndex>>,

So what we can do is to modify each_borrow_involving_path. Instead of taking an iterator over borrows in scope, it would take some sort of is_borrow_in_scope: impl Fn(BorrowIndex) -> bool function. It would then find the root local (if any) of the access path, find all borrows that apply to that root local, and iterate over those, testing if each of them is in scope.

We could make this more efficient in many ways -- e.g., using bitsets and things -- but this basic change should already eliminate some of the horrible O(n^2) behavior.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-nll NLL-performant Working towards the "performance is good" goal labels Aug 7, 2018
@nikomatsakis nikomatsakis added this to the Rust 2018 RC milestone Aug 7, 2018
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Aug 7, 2018

This should be a big win for html5ever -- each_borrow_involving_path currently stands at 41% of borrowck time there!

@nikomatsakis
Copy link
Contributor Author

This will likely not help html5ever as much once #53177 lands.

#53189 is also relevant, as it will also remove many uses of each_borrow_involving_path.

@mikhail-m1 mikhail-m1 self-assigned this Aug 8, 2018
@nikomatsakis
Copy link
Contributor Author

@Mark-Simulacrum
Copy link
Member

Moving to RC 2 -- not a blocker.

@nikomatsakis
Copy link
Contributor Author

Removing from all milestones as this just doesn't release like a Rust 2018 blocker (perf is "good enough"). Marked as NLL-deferred.

@pnkfelix pnkfelix added the WG-compiler-performance Working group: Compiler Performance label Feb 19, 2019
@pnkfelix
Copy link
Member

NLL premeeting triage. Unclear what priority to assign here, since it is not clear whether or not this would still be much of a win. (Its possible it might be; I just don't know.) Continuing to leave off a P-label; hopfeully I'll revisit next week with more information.

@nikomatsakis
Copy link
Contributor Author

I'm inclined to just close this issue, personally, although I guess the idea may still be actionable and relevant.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 6, 2019

NLL triage: closing. If we later decide that this idea has merit, we can reopen (or open a fresh issue, or just implement a PR).

@pnkfelix pnkfelix closed this as completed Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-performant Working towards the "performance is good" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-performance Working group: Compiler Performance
Projects
None yet
Development

No branches or pull requests

5 participants