optimize NLL constraint propagation a little #49472

nikomatsakis · 2018-03-29T07:42:05Z

Removes a bone-headed hot spot in NLL constant propagation; we were re-allocating the stack vector and hashmap as we repeated the DFS. This change shares those resources across each call.

It also modifies the constraint list to be a linked list; arguably I should revert that, though, as this didn't turn out to be a perf hit and perhaps the old code was clearer? (Still, the new style appeals to me.)

r? @pnkfelix

Mark-Simulacrum · 2018-03-29T12:36:45Z

@bors try

Mark-Simulacrum · 2018-03-29T08:20:51Z

src/librustc_mir/borrow_check/nll/region_infer/dfs.rs

@@ -34,25 +51,29 @@ impl<'tcx> RegionInferenceContext<'tcx> {
    /// - `Ok(false)` if the walk was completed with no changes;
    /// - `Err(early)` if the walk was existed early by `op`. `earlyelem` is the
    ///   value that `op` returned.
-    pub(super) fn dfs<C>(&self, mir: &Mir<'tcx>, mut op: C) -> Result<bool, C::Early>
+    #[inline(never)] // ensure dfs is identifiable in profiles


Should we keep this in? Seems potentially bad for performance, though it does seem somewhat unlikely.

(removed comment from review that was intended as response to @Mark-Simulacrum above)

I don't believe this would ever be criticial for performance, this is why I kept it in, but for sure it is not relevant now, given the data we're looking at. =) And it's convenient for profiling etc.

bors · 2018-03-29T12:36:56Z

⌛ Trying commit 8144c8f with merge 288ea68...

@pnkfelix

…=<try> optimize NLL constraint propagation a little Removes a bone-headed hot spot in NLL constant propagation; we were re-allocating the stack vector and hashmap as we repeated the DFS. This change shares those resources across each call. It also modifies the constraint list to be a linked list; arguably I should revert that, though, as this didn't turn out to be a perf hit and perhaps the old code was clearer? (Still, the new style appeals to me.) r? @pnkfelix

kennytm · 2018-03-29T12:52:14Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

@@ -539,7 +587,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
        for type_test in &self.type_tests {
            debug!("check_type_test: {:?}", type_test);

-            if self.eval_region_test(mir, type_test.point, type_test.lower_bound, &type_test.test) {
+            if self.eval_region_test(mir, dfs_storage, type_test.point, type_test.lower_bound, &type_test.test) {


[00:05:33] tidy error: /checkout/src/librustc_mir/borrow_check/nll/region_infer/mod.rs:590: line longer than 100 chars [00:05:34] some tidy checks failed

pnkfelix

r=me after you've addressed the nits.

pnkfelix · 2018-03-29T14:07:04Z

src/librustc_mir/borrow_check/nll/region_infer/dfs.rs

@@ -34,25 +51,29 @@ impl<'tcx> RegionInferenceContext<'tcx> {
    /// - `Ok(false)` if the walk was completed with no changes;
    /// - `Err(early)` if the walk was existed early by `op`. `earlyelem` is the
    ///   value that `op` returned.
-    pub(super) fn dfs<C>(&self, mir: &Mir<'tcx>, mut op: C) -> Result<bool, C::Early>
+    #[inline(never)] // ensure dfs is identifiable in profiles


(removed comment from review that was intended as response to @Mark-Simulacrum above)

pnkfelix · 2018-03-29T14:16:00Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

@@ -143,10 +148,22 @@ pub struct Constraint {
    /// At this location.
    point: Location,

+    /// Later on, we thread the constraints onto a linked list
+    /// sorted by their `sub` field. So if you had:


The use of the word "sort" in this comment confused me: "There's a single list and its sorted by the sub field? Or there's a bunch of lists and each is individually sorted by the sub field -- but how is there any kind of sort order here?"

I usually treat the term "sort" as meaning "make an ordered sequence"; but here you mean it more as "group together (via a linked list) all of the constraints that have the same sub field."

pnkfelix · 2018-03-29T14:18:49Z

src/librustc_mir/borrow_check/nll/region_infer/mod.rs

@@ -403,10 +422,23 @@ impl<'tcx> RegionInferenceContext<'tcx> {
        infcx: &InferCtxt<'_, 'gcx, 'tcx>,
        mir: &Mir<'tcx>,
        mir_def_id: DefId,
+    ) -> Option<ClosureRegionRequirements<'gcx>> {
+        common::time(infcx.tcx.sess, &format!("solve({:?})", mir_def_id), || {


Consider using something a little more specific than just "solve" here, like "solve region constraints", just to ease lives of other users of -Z time-passes

bors · 2018-03-29T14:23:10Z

☀️ Test successful - status-travis
State: approved= try=True

nikomatsakis · 2018-03-29T15:29:35Z

@Mark-Simulacrum try run succeeded, were you gonna do a perf run?

Mark-Simulacrum · 2018-03-29T17:12:19Z

Totally forgot, but I've started it now. We can land without too, I don't particularly care.

nikomatsakis · 2018-03-29T20:51:57Z

@bors r=pnkfelix

bors · 2018-03-29T20:51:57Z

📌 Commit ca8176d has been approved by pnkfelix

nikomatsakis · 2018-03-29T20:52:18Z

@Mark-Simulacrum as usual, I am unable to construct a working URL :(

This is what I tried:

http://perf.rust-lang.org/compare.html?start=288ea682772125d25d7c076ba0b7d87584da1189&end=409744bcb91f4efa35b8fcc9e7033523a86b90c2&stat=instructions:u

But I know it's gonna' be faster.

UPDATE: Oh, hey, it works now! But I got the order of the revisions backward. Try this:

http://perf.rust-lang.org/compare.html?start=409744bcb91f4efa35b8fcc9e7033523a86b90c2&end=288ea682772125d25d7c076ba0b7d87584da1189&stat=instructions:u

bors · 2018-03-31T04:09:49Z

⌛ Testing commit ca8176d with merge 3c5f850...

@pnkfelix

…=pnkfelix optimize NLL constraint propagation a little Removes a bone-headed hot spot in NLL constant propagation; we were re-allocating the stack vector and hashmap as we repeated the DFS. This change shares those resources across each call. It also modifies the constraint list to be a linked list; arguably I should revert that, though, as this didn't turn out to be a perf hit and perhaps the old code was clearer? (Still, the new style appeals to me.) r? @pnkfelix

bors · 2018-03-31T06:40:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 3c5f850 to master...

rust-highfive assigned pnkfelix Mar 29, 2018

add #[inline(never)] annotations

1ef1b92

nikomatsakis force-pushed the nll-optimize-constraint-prop-1 branch from a8a09b4 to 8144c8f Compare March 29, 2018 07:45

Mark-Simulacrum reviewed Mar 29, 2018

View reviewed changes

kennytm reviewed Mar 29, 2018

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2018

pnkfelix approved these changes Mar 29, 2018

View reviewed changes

nikomatsakis added 4 commits March 29, 2018 13:32

remove dependency map and instead use a linked list of constraints

96f3e56

amortize dfs storage creation

499d784

document reason for #[inline(never)] annotation

bd90302

apply pnkfelix nits

ca8176d

nikomatsakis force-pushed the nll-optimize-constraint-prop-1 branch from 551d512 to ca8176d Compare March 29, 2018 17:32

bors added S-waiting-on-bors 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 Mar 29, 2018

bors merged commit ca8176d into rust-lang:master Mar 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize NLL constraint propagation a little #49472

optimize NLL constraint propagation a little #49472

nikomatsakis commented Mar 29, 2018

Mark-Simulacrum commented Mar 29, 2018

Mark-Simulacrum Mar 29, 2018

pnkfelix Mar 29, 2018 •

edited

Loading

nikomatsakis Mar 29, 2018

bors commented Mar 29, 2018

kennytm Mar 29, 2018

pnkfelix left a comment

pnkfelix Mar 29, 2018 •

edited

Loading

pnkfelix Mar 29, 2018

pnkfelix Mar 29, 2018

bors commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018

Mark-Simulacrum commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018

bors commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018 •

edited

Loading

bors commented Mar 31, 2018

bors commented Mar 31, 2018

optimize NLL constraint propagation a little #49472

optimize NLL constraint propagation a little #49472

Conversation

nikomatsakis commented Mar 29, 2018

Mark-Simulacrum commented Mar 29, 2018

Mark-Simulacrum Mar 29, 2018

Choose a reason for hiding this comment

pnkfelix Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Mar 29, 2018

Choose a reason for hiding this comment

bors commented Mar 29, 2018

kennytm Mar 29, 2018

Choose a reason for hiding this comment

pnkfelix left a comment

Choose a reason for hiding this comment

pnkfelix Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

pnkfelix Mar 29, 2018

Choose a reason for hiding this comment

pnkfelix Mar 29, 2018

Choose a reason for hiding this comment

bors commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018

Mark-Simulacrum commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018

bors commented Mar 29, 2018

nikomatsakis commented Mar 29, 2018 • edited Loading

bors commented Mar 31, 2018

bors commented Mar 31, 2018

pnkfelix Mar 29, 2018 •

edited

Loading

pnkfelix Mar 29, 2018 •

edited

Loading

nikomatsakis commented Mar 29, 2018 •

edited

Loading