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

Remove `NodeState::{Waiting,Done}` #66405

Merged
merged 5 commits into from Dec 13, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Nov 14, 2019

An optimization, and then some clean-ups.

r? @nikomatsakis

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 24, 2019

Ping from triage
@nikomatsakis can you please review this PR?
Thanks

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 2, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 2, 2019

📌 Commit c45fc6b has been approved by nikomatsakis

Centril added a commit to Centril/rust that referenced this pull request Dec 3, 2019
…tates, r=nikomatsakis

Tweak `ObligationForest` `NodeState`s

These two commits improve comments and function names.

r? @nikomatsakis
bors added a commit that referenced this pull request Dec 3, 2019
Rollup of 9 pull requests

Successful merges:

 - #66405 (Tweak `ObligationForest` `NodeState`s)
 - #66651 (Add `enclosing scope` parameter to `rustc_on_unimplemented`)
 - #66730 (remove dependency from libhermit)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66904 (Adding docs for keyword match, move)
 - #66927 (Miri core engine: use throw_ub instead of throw_panic)
 - #66935 (syntax: Unify macro and attribute arguments in AST)
 - #66941 (Remove `ord` lang item)
 - #66967 (Remove hack for top-level or-patterns in match checking)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Dec 3, 2019
Rollup of 9 pull requests

Successful merges:

 - #66405 (Tweak `ObligationForest` `NodeState`s)
 - #66651 (Add `enclosing scope` parameter to `rustc_on_unimplemented`)
 - #66730 (remove dependency from libhermit)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66904 (Adding docs for keyword match, move)
 - #66927 (Miri core engine: use throw_ub instead of throw_panic)
 - #66935 (syntax: Unify macro and attribute arguments in AST)
 - #66941 (Remove `ord` lang item)
 - #66967 (Remove hack for top-level or-patterns in match checking)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 5, 2019
…tates, r=nikomatsakis

Tweak `ObligationForest` `NodeState`s

These two commits improve comments and function names.

r? @nikomatsakis
bors added a commit that referenced this pull request Dec 5, 2019
Rollup of 13 pull requests

Successful merges:

 - #66405 (Tweak `ObligationForest` `NodeState`s)
 - #66730 (remove dependency from libhermit)
 - #66764 (Tweak wording of `collect()` on bad target type)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66900 (Clean up error codes)
 - #66933 (Fix pointing at arg when cause is outside of call)
 - #66952 (Use Module::print() instead of a PrintModulePass)
 - #66979 (Add long error for E0631 and update ui tests.)
 - #67005 (capitalize Rust)
 - #67010 (Accurately portray raw identifiers in error messages)
 - #67011 (Include a span in more `expected...found` notes)
 - #67017 (cleanup long error explanations)
 - #67021 (Fix docs for formatting delegations)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 5, 2019
…tates, r=nikomatsakis

Tweak `ObligationForest` `NodeState`s

These two commits improve comments and function names.

r? @nikomatsakis
bors added a commit that referenced this pull request Dec 5, 2019
Rollup of 13 pull requests

Successful merges:

 - #66405 (Tweak `ObligationForest` `NodeState`s)
 - #66730 (remove dependency from libhermit)
 - #66764 (Tweak wording of `collect()` on bad target type)
 - #66828 (Less minification)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66900 (Clean up error codes)
 - #66952 (Use Module::print() instead of a PrintModulePass)
 - #66979 (Add long error for E0631 and update ui tests.)
 - #67005 (capitalize Rust)
 - #67010 (Accurately portray raw identifiers in error messages)
 - #67011 (Include a span in more `expected...found` notes)
 - #67017 (cleanup long error explanations)
 - #67021 (Fix docs for formatting delegations)

Failed merges:

r? @ghost
RalfJung added a commit to RalfJung/rust that referenced this pull request Dec 5, 2019
…tates, r=nikomatsakis

Tweak `ObligationForest` `NodeState`s

These two commits improve comments and function names.

r? @nikomatsakis
bors added a commit that referenced this pull request Dec 5, 2019
Rollup of 13 pull requests

Successful merges:

 - #66405 (Tweak `ObligationForest` `NodeState`s)
 - #66730 (remove dependency from libhermit)
 - #66764 (Tweak wording of `collect()` on bad target type)
 - #66899 (Standard library support for riscv64gc-unknown-linux-gnu)
 - #66900 (Clean up error codes)
 - #66974 ([CI] fix the `! isCI` check in src/ci/run.sh)
 - #66979 (Add long error for E0631 and update ui tests.)
 - #67005 (capitalize Rust)
 - #67010 (Accurately portray raw identifiers in error messages)
 - #67011 (Include a span in more `expected...found` notes)
 - #67017 (cleanup long error explanations)
 - #67021 (Fix docs for formatting delegations)
 - #67041 (add ExitStatusExt into prelude)

Failed merges:

r? @ghost
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 6, 2019

Actually, I'm going to do some more changes here.

@bors r-

@nnethercote nnethercote force-pushed the nnethercote:tweak-ObligForest-NodeStates branch from c45fc6b to 3a5b4ed Dec 6, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 6, 2019

The new commits subsume the old ones.

r? @nikomatsakis

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 6, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 6, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 6, 2019

⌛️ Trying commit 3a5b4ed with merge 0e8c241...

bors added a commit that referenced this pull request Dec 6, 2019
Tweak `ObligationForest` `NodeState`s

An optimization, and then some clean-ups.

r? @nikomatsakis
@nnethercote nnethercote force-pushed the nnethercote:tweak-ObligForest-NodeStates branch from 829fd7c to cb21293 Dec 12, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 12, 2019

I rebased. Let's try another perf CI run, see if the results have changed :/

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 12, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 12, 2019

⌛️ Trying commit cb21293 with merge 88ae36b...

bors added a commit that referenced this pull request Dec 12, 2019
Remove `NodeState::{Waiting,Done}`

An optimization, and then some clean-ups.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 13, 2019

☀️ Try build successful - checks-azure
Build commit: 88ae36b (88ae36bd20175c16667f51a71fbedcae8f933c44)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 13, 2019

Queued 88ae36b with parent e9469a6, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Dec 13, 2019

Finished benchmarking try commit 88ae36b, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 13, 2019

The new results still don't really match what I got locally, but they do show it as a small win, so I'm going to land this.

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 13, 2019

📌 Commit cb21293 has been approved by nikomatsakis

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 13, 2019

@bors rollup=never

(The rollup directive above was from an earlier version of this PR when the changes were much simpler.)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 13, 2019

⌛️ Testing commit cb21293 with merge 9409c20...

bors added a commit that referenced this pull request Dec 13, 2019
…komatsakis

Remove `NodeState::{Waiting,Done}`

An optimization, and then some clean-ups.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 13, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 9409c20 to master...

@bors bors added the merged-by-bors label Dec 13, 2019
@bors bors merged commit cb21293 into rust-lang:master Dec 13, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr #20191212.44 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:tweak-ObligForest-NodeStates branch Dec 15, 2019
@sdroege

This comment has been minimized.

Copy link
Contributor

sdroege commented Dec 20, 2019

This seems to have caused #67454, which is maybe a duplicate of #67331

nnethercote added a commit to nnethercote/rust that referenced this pull request Dec 21, 2019
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.
nnethercote added a commit to nnethercote/rust that referenced this pull request Dec 21, 2019
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes rust-lang#67454.
bors added a commit that referenced this pull request Dec 21, 2019
Revert parts of #66405.

Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes #67454.

r? @nikomatsakis
Mark-Simulacrum added a commit to nnethercote/rust that referenced this pull request Dec 31, 2019
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes rust-lang#67454.
Mark-Simulacrum added a commit to nnethercote/rust that referenced this pull request Dec 31, 2019
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes rust-lang#67454.
bors added a commit that referenced this pull request Jan 1, 2020
Revert parts of #66405.

Because PR #66405 caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes #67454.

r? @nikomatsakis
Marwes added a commit to Marwes/rust that referenced this pull request Jan 3, 2020
This was lost in rust-lang#66405 , perhaps
due to the added `dep_index >= min_index` check which attempts to avoid
reprocessing in a different way. Unfortunately that does not cover the
case where a node points to a higher index which can in degenerate cases
where a lot of nodes point to a lot of higher indexed nodes end up with
`O(N ^ 2)` complexity. With this fix restored it is `O(N)` again since
nodes will never be reprocessed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.