Various `ObligationForest` improvements #64500

nnethercote · 2019-09-16T06:40:33Z

These commits make the code both nicer and faster.

Those with type `usize` are now called `i`, those with type `NodeIndex` are called `index`.

This commit removes the custom index implementation of `NodeIndex`, which probably predates `newtype_index!`. As well as eliminating code, it improves the debugging experience, because the custom implementation had the property of being incremented by 1 (so it could use `NonZeroU32`), which was incredibly confusing if you didn't expect it. For some reason, I also had to remove an `unsafe` block marker from `from_u32_unchecked()` that the compiler said was now unnecessary.

These refer to code that no longer exists.

This makes the code a little faster, presumably because bounds checks aren't needed on `nodes` accesses. It requires making `scratch` a `RefCell`, which is not unreasonable.

It's more concise, more idiomatic, and measurably faster.

nnethercote · 2019-09-16T07:21:57Z

I will do a perf run.

@bors try

bors · 2019-09-16T07:22:09Z

⌛ Trying commit 4ecd94e with merge b9036784da288d61ad41be8bd937c6d333c7a8bd...

bors · 2019-09-16T10:23:24Z

☀️ Try build successful - checks-azure
Build commit: b9036784da288d61ad41be8bd937c6d333c7a8bd

Centril · 2019-09-16T13:01:25Z

@rust-timer build b9036784da288d61ad41be8bd937c6d333c7a8bd

rust-timer · 2019-09-16T13:01:27Z

Success: Queued b9036784da288d61ad41be8bd937c6d333c7a8bd with parent 3e3e06d, comparison URL.

rust-timer · 2019-09-16T14:55:08Z

Finished benchmarking try commit b9036784da288d61ad41be8bd937c6d333c7a8bd, comparison URL.

nikomatsakis · 2019-09-16T20:56:22Z

src/librustc_data_structures/indexed_vec.rs

@@ -149,7 +149,7 @@ macro_rules! newtype_index {

            #[inline]
            $v const unsafe fn from_u32_unchecked(value: u32) -> Self {
-                unsafe { $type { private: value } }
+                $type { private: value }


it's not needed because the function itself is unsafe, so it's allowed to have unsafe operations in the body

Fair enough. The obvious follow-up question is "why didn't the compiler complain about this before?"

nikomatsakis · 2019-09-16T21:04:01Z

r=me once the benchmarking results are in.

Centril · 2019-09-16T21:06:14Z

@nikomatsakis the bench results are already in... :D

nikomatsakis · 2019-09-16T21:45:22Z

Oh, so they are.

@bors r+

bors · 2019-09-16T21:45:23Z

📌 Commit 4ecd94e has been approved by nikomatsakis

@nikomatsakis

…ikomatsakis Various `ObligationForest` improvements These commits make the code both nicer and faster. r? @nikomatsakis

@ghost

Rollup of 6 pull requests Successful merges: - #64085 (Tweak unsatisfied HRTB errors) - #64380 (Update bundled OpenSSL to 1.1.1d) - #64416 (Various refactorings to clean up nll diagnostics) - #64500 (Various `ObligationForest` improvements) - #64530 (Elide lifetimes in `Pin<&(mut) Self>`) - #64531 (Use shorthand syntax in the self parameter of methods of Pin) Failed merges: r? @ghost

nnethercote · 2019-09-17T03:32:37Z

Thank you for the fast review.

BTW, this code can be very hot and parts of it are very perf-sensitive to change. Here is a list of other ways I tried to speed up or clean up ObligationForest, but failed:

Inline and remove insert_into_error_cache again(); caused slow-downs even when that code wasn't executed, presumably due to different inlining?
Use std::usize::MAX to indicate nodes to be removed; caused slow-downs.
Use IndexVec<Node<O>> for nodes; caused slow-downs.
Use drain_filter() for dependents in apply_rewrites(); caused slow-downs.
Use drain_filter() in compress; couldn't get past the borrow checker.
Make process_obligation() iterate over new nodes added to nodes; caused infinite loops.
Split nodes in two, to reduce the number of bytes copied by the shuffling in compress(); didn't finish, felt too invasive and likely to cause slow-downs.
Use swap_and_remove() to reduce the number of bytes copied by the shuffling in `compress(); caused slow-downs.
Box everything in Node; caused slow-downs.
Box everything in Node except state: caused slow-downs

@nikomatsakis

More `ObligationForest` improvements Following on from #64500, these commits alsomake the code both nicer and faster. r? @nikomatsakis

sinkuu · 2019-09-19T01:09:54Z

src/librustc_data_structures/obligation_forest/mod.rs

-    scratch: Option<Vec<usize>>,
+    /// A scratch vector reused in various operations, to avoid allocating new
+    /// vectors.
+    scratch: RefCell<Vec<usize>>,


I believe this can be replaced with Cell, which is simpler and panic-free, as you are only calling .replace on this (i.e. not utilizing "Ref" functionality).

Thanks for the tip. I will do it in a follow-up.

@nikomatsakis

More `ObligationForest` improvements Following on from #64500, these commits alsomake the code both nicer and faster. r? @nikomatsakis

nnethercote added 11 commits September 16, 2019 11:52

Name index variables consistently.

04b1111

Those with type `usize` are now called `i`, those with type `NodeIndex` are called `index`.

Factor out repeated self.nodes[i] expressions.

1936e44

Remove out-of-date comments.

3fda957

These refer to code that no longer exists.

Fix some out-of-date names of things in comments.

ac061dc

Fix incorrect comment about contents of a Node.

6391ef4

Add comments about waiting_cache.

e2492b7

Use iterators in error_at and process_cycle.

6e48053

This makes the code a little faster, presumably because bounds checks aren't needed on `nodes` accesses. It requires making `scratch` a `RefCell`, which is not unreasonable.

Use retain for waiting_cache in apply_rewrites().

f22bb2e

It's more concise, more idiomatic, and measurably faster.

Minor comment tweaks.

201afa6

Move impl Node just after struct Node.

4ecd94e

rust-highfive assigned nikomatsakis Sep 16, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2019

nnethercote changed the title ~~Various ObligationForest~~ Various ObligationForest improvements Sep 16, 2019

nikomatsakis reviewed Sep 16, 2019

View reviewed changes

nikomatsakis approved these changes Sep 16, 2019

View reviewed changes

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 Sep 16, 2019

Centril added a commit to Centril/rust that referenced this pull request Sep 17, 2019

Rollup merge of rust-lang#64500 - nnethercote:ObligForest-fixups, r=n…

1e3b57e

…ikomatsakis Various `ObligationForest` improvements These commits make the code both nicer and faster. r? @nikomatsakis

Centril mentioned this pull request Sep 17, 2019

Rollup of 6 pull requests #64535

Merged

bors merged commit 4ecd94e into rust-lang:master Sep 17, 2019

nnethercote deleted the ObligForest-fixups branch September 17, 2019 05:25

nnethercote mentioned this pull request Sep 17, 2019

More ObligationForest improvements #64545

Merged

bors added a commit that referenced this pull request Sep 17, 2019

Auto merge of #64545 - nnethercote:ObligForest-more, r=<try>

2ae267a

More `ObligationForest` improvements Following on from #64500, these commits alsomake the code both nicer and faster. r? @nikomatsakis

sinkuu reviewed Sep 19, 2019

View reviewed changes

bors added a commit that referenced this pull request Sep 19, 2019

Auto merge of #64545 - nnethercote:ObligForest-more, r=nmatsakis

9b9d2af

More `ObligationForest` improvements Following on from #64500, these commits alsomake the code both nicer and faster. r? @nikomatsakis

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various `ObligationForest` improvements #64500

Various `ObligationForest` improvements #64500

nnethercote commented Sep 16, 2019

nnethercote commented Sep 16, 2019

bors commented Sep 16, 2019

bors commented Sep 16, 2019

Centril commented Sep 16, 2019

rust-timer commented Sep 16, 2019

rust-timer commented Sep 16, 2019

nikomatsakis Sep 16, 2019

nnethercote Sep 16, 2019

nikomatsakis commented Sep 16, 2019

Centril commented Sep 16, 2019

nikomatsakis commented Sep 16, 2019

bors commented Sep 16, 2019

nnethercote commented Sep 17, 2019

sinkuu Sep 19, 2019

nnethercote Sep 19, 2019

Various ObligationForest improvements #64500

Various ObligationForest improvements #64500

Conversation

nnethercote commented Sep 16, 2019

nnethercote commented Sep 16, 2019

bors commented Sep 16, 2019

bors commented Sep 16, 2019

Centril commented Sep 16, 2019

rust-timer commented Sep 16, 2019

rust-timer commented Sep 16, 2019

nikomatsakis Sep 16, 2019

Choose a reason for hiding this comment

nnethercote Sep 16, 2019

Choose a reason for hiding this comment

nikomatsakis commented Sep 16, 2019

Centril commented Sep 16, 2019

nikomatsakis commented Sep 16, 2019

bors commented Sep 16, 2019

nnethercote commented Sep 17, 2019

sinkuu Sep 19, 2019

Choose a reason for hiding this comment

nnethercote Sep 19, 2019

Choose a reason for hiding this comment

Various `ObligationForest` improvements #64500

Various `ObligationForest` improvements #64500