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

Make `process_obligations()` greedier. #66408

Merged
merged 1 commit into from Dec 4, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Nov 14, 2019

process_obligations() adds new nodes, but it does not process these
new nodes until the next time it is called.

This commit changes it so that it does process these new nodes within
the same call. This change reduces the number of calls to
process_obligations() required to complete processing, sometimes
giving significant speed-ups.

The change required some changes to tests.

  • The output of cycle-cache-err-60010.rs is slightly different.
  • The unit tests required extra cases to handle the earlier processing
    of the added nodes. I mostly did these in the simplest possible way,
    by making the added nodes be ignored, thus giving outcomes the same as
    with the old behaviour. But I changed success_in_grandchildren()
    more extensively so that some obligations are completed earlier than
    they used to be.

r? @nikomatsakis

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 14, 2019

@nikomatakis: I'm not 100% confident about the cycle-cache-err-60010.rs output changes, but I'm guessing/hoping that the new output is similar enough that it's equivalent.

Some local instruction count measurements:

keccak-check
        avg: -4.9%      min: -7.5%      max: -0.0%
inflate-check
        avg: -2.6%      min: -3.6%      max: -0.0%
cranelift-codegen-check
        avg: -1.2%      min: -2.0%      max: 0.0%
wg-grammar-check
        avg: -1.2%      min: -1.8%      max: -0.1%
clap-rs-check
        avg: -0.7%      min: -1.7%      max: 0.0%
unify-linearly-check
        avg: -0.4%      min: -0.7%      max: -0.1%
deeply-nested-check
        avg: -0.4%      min: -0.7%      max: -0.1%
regression-31157-check
        avg: -0.2%      min: -0.5%      max: -0.0%
style-servo-check
        avg: -0.2%      min: -0.4%      max: -0.0%
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 14, 2019

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 14, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 14, 2019

⌛️ Trying commit 98a7454 with merge f988482...

bors added a commit that referenced this pull request Nov 14, 2019
Make `process_obligations()` greedier.

`process_obligations()` adds new nodes, but it does not process these
new nodes until the next time it is called.

This commit changes it so that it does process these new nodes within
the same call. This change reduces the number of calls to
`process_obligations()` required to complete processing, sometimes
giving significant speed-ups.

The change required some changes to tests.
- The output of `cycle-cache-err-60010.rs` is slightly different.
- The unit tests required extra cases to handle the earlier processing
  of the added nodes. I mostly did these in the simplest possible way,
  by making the added nodes be ignored, thus giving outcomes the same as
  with the old behaviour. But I changed `success_in_grandchildren()`
  more extensively so that some obligations are completed earlier than
  they used to be.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 14, 2019

☀️ Try build successful - checks-azure
Build commit: f988482 (f988482f08119a7de8f3a4a18c101dd23b5eba1a)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Nov 14, 2019

Queued f988482 with parent 5e380b7, future comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Nov 14, 2019

The perf improvements on CI are similar to what I saw locally.

`process_obligations()` adds new nodes, but it does not process these
new nodes until the next time it is called.

This commit changes it so that it does process these new nodes within
the same call. This change reduces the number of calls to
`process_obligations()` required to complete processing, sometimes
giving significant speed-ups.

The change required some changes to tests.
- The output of `cycle-cache-err-60010.rs` is slightly different.
- The unit tests required extra cases to handle the earlier processing
  of the added nodes. I mostly did these in the simplest possible way,
  by making the added nodes be ignored, thus giving outcomes the same as
  with the old behaviour. But I changed `success_in_grandchildren()`
  more extensively so that some obligations are completed earlier than
  they used to be.
@nnethercote nnethercote force-pushed the nnethercote:greedy-process_obligations branch from 98a7454 to 05f13a8 Nov 14, 2019
@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 24, 2019

Ping from triage
@nikomatsakis - can you please review this PR?
Thank you!

Copy link
Contributor

nikomatsakis left a comment

r=me modulo nit below -- @nnethercote if you hate it you can also ignore, but it seems like a slight improvement to me

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Dec 3, 2019

@bors r=nmatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 3, 2019

📌 Commit 05f13a8 has been approved by nmatsakis

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 3, 2019

@bors rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 3, 2019

⌛️ Testing commit 05f13a8 with merge 0ecfb93...

bors added a commit that referenced this pull request Dec 3, 2019
…sakis

Make `process_obligations()` greedier.

`process_obligations()` adds new nodes, but it does not process these
new nodes until the next time it is called.

This commit changes it so that it does process these new nodes within
the same call. This change reduces the number of calls to
`process_obligations()` required to complete processing, sometimes
giving significant speed-ups.

The change required some changes to tests.
- The output of `cycle-cache-err-60010.rs` is slightly different.
- The unit tests required extra cases to handle the earlier processing
  of the added nodes. I mostly did these in the simplest possible way,
  by making the added nodes be ignored, thus giving outcomes the same as
  with the old behaviour. But I changed `success_in_grandchildren()`
  more extensively so that some obligations are completed earlier than
  they used to be.

r? @nikomatsakis
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 3, 2019

@bors retry yielding to r0llup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

⌛️ Testing commit 05f13a8 with merge c4f1304...

bors added a commit that referenced this pull request Dec 4, 2019
…sakis

Make `process_obligations()` greedier.

`process_obligations()` adds new nodes, but it does not process these
new nodes until the next time it is called.

This commit changes it so that it does process these new nodes within
the same call. This change reduces the number of calls to
`process_obligations()` required to complete processing, sometimes
giving significant speed-ups.

The change required some changes to tests.
- The output of `cycle-cache-err-60010.rs` is slightly different.
- The unit tests required extra cases to handle the earlier processing
  of the added nodes. I mostly did these in the simplest possible way,
  by making the added nodes be ignored, thus giving outcomes the same as
  with the old behaviour. But I changed `success_in_grandchildren()`
  more extensively so that some obligations are completed earlier than
  they used to be.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2019

☀️ Test successful - checks-azure
Approved by: nmatsakis
Pushing c4f1304 to master...

@bors bors added the merged-by-bors label Dec 4, 2019
@bors bors merged commit 05f13a8 into rust-lang:master Dec 4, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191114.55 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 (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:greedy-process_obligations branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.