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

Various `ObligationForest` improvements #64500

Merged
merged 11 commits into from Sep 17, 2019

Conversation

@nnethercote
Copy link
Contributor

commented Sep 16, 2019

These commits make the code both nicer and faster.

r? @nikomatsakis

nnethercote added 11 commits Sep 16, 2019
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 nnethercote changed the title Various `ObligationForest` Various `ObligationForest` improvements Sep 16, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

I will do a perf run.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

⌛️ Trying commit 4ecd94e with merge b903678...

bors added a commit that referenced this pull request Sep 16, 2019
Various `ObligationForest` improvements

These commits make the code both nicer and faster.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

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

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

@rust-timer

This comment has been minimized.

Copy link

commented Sep 16, 2019

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

@rust-timer

This comment has been minimized.

Copy link

commented Sep 16, 2019

Finished benchmarking try commit b903678, comparison URL.

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

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Sep 16, 2019

Contributor

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

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 16, 2019

Author Contributor

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

r=me once the benchmarking results are in.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Oh, so they are.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

📌 Commit 4ecd94e has been approved by nikomatsakis

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

Various `ObligationForest` improvements

These commits make the code both nicer and faster.

r? @nikomatsakis
bors added a commit that referenced this pull request Sep 17, 2019
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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

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
@bors bors merged commit 4ecd94e into rust-lang:master Sep 17, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20190916.9 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:ObligForest-fixups branch Sep 17, 2019
bors added a commit that referenced this pull request Sep 17, 2019
More `ObligationForest` improvements

Following on from #64500, these commits alsomake the code both nicer and faster.

r? @nikomatsakis
scratch: Option<Vec<usize>>,
/// A scratch vector reused in various operations, to avoid allocating new
/// vectors.
scratch: RefCell<Vec<usize>>,

This comment has been minimized.

Copy link
@sinkuu

sinkuu Sep 19, 2019

Contributor

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

This comment has been minimized.

Copy link
@nnethercote

nnethercote Sep 19, 2019

Author Contributor

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

bors added a commit that referenced this pull request Sep 19, 2019
More `ObligationForest` improvements

Following on from #64500, these commits alsomake the code both nicer and faster.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.