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

Jump to bottom

Optimize shallow_resolve_changed #67079

Merged
merged 2 commits into from Dec 12, 2019
Merged

Optimize shallow_resolve_changed #67079

merged 2 commits into from Dec 12, 2019

Conversation

nnethercote
Copy link
Contributor

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2019
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 6, 2019

⌛ Trying commit 01025ef6e9babe54a3dad067d878c87deaf55a31 with merge d4119bbbe06d9b4edebef7f71ab6acafff5d0bfa...

@bors
Copy link
Contributor

bors commented Dec 6, 2019

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

@rust-timer
Copy link
Collaborator

Queued d4119bbbe06d9b4edebef7f71ab6acafff5d0bfa with parent 234c9f2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit d4119bbbe06d9b4edebef7f71ab6acafff5d0bfa, comparison URL.

@nikomatsakis
Copy link
Contributor

Seems like it's not really a win, but I'm not opposed to landing it.

@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

Seems like a net loss, so why land it?

@nnethercote
Copy link
Contributor Author

That's weird... it was a clear win of about 3-4% on inflate-check-clean and keccak-check-clean on my machine. I will investigate on Monday.

@nnethercote nnethercote force-pushed the optimize-shallow_resolve_changed branch from 01025ef to 0cb6b45 Compare December 9, 2019 01:52
@nnethercote
Copy link
Contributor Author

I rebased and double-checked the results. Here's what I get locally:

inflate-check
        avg: -4.2%      min: -4.2%      max: -4.2%
keccak-check
        avg: -3.0%      min: -3.0%      max: -3.0%
cranelift-codegen-check
        avg: -0.4%      min: -0.4%      max: -0.4%
clap-rs-check
        avg: -0.3%      min: -0.3%      max: -0.3%
serde-check
        avg: -0.3%      min: -0.3%      max: -0.3%
deeply-nested-check
        avg: -0.2%      min: -0.2%      max: -0.2%
unify-linearly-check
        avg: -0.2%      min: -0.2%      max: -0.2%
wg-grammar-check
        avg: -0.1%      min: -0.1%      max: -0.1%
style-servo-check
        avg: -0.1%      min: -0.1%      max: -0.1%
syn-check
        avg: -0.1%      min: -0.1%      max: -0.1%
deep-vector-check
        avg: -0.1%      min: -0.1%      max: -0.1%
piston-image-check
        avg: -0.1%      min: -0.1%      max: -0.1%
cargo-check
        avg: -0.1%      min: -0.1%      max: -0.1%

Let's try again.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 9, 2019

⌛ Trying commit 0cb6b45 with merge ec4d9e0...

bors added a commit that referenced this pull request Dec 9, 2019
@bors
Auto merge of #67079 - nnethercote:optimize-shallow_resolve_changed, …
ec4d9e0 
…r=<try>

Optimize `shallow_resolve_changed`

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 9, 2019

☀️ Try build successful - checks-azure
Build commit: ec4d9e0 (ec4d9e01ff378edf7bc46f5d3d0158988ee8fd82)

@rust-timer
Copy link
Collaborator

Queued ec4d9e0 with parent 59947fc, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ec4d9e0, comparison URL.

@nnethercote
Copy link
Contributor Author

Finished benchmarking try commit ec4d9e0, comparison URL.

@Mark-Simulacrum: no data is being shown in the results. (Likewise in #66405.) Any idea what's wrong?

@nnethercote
Copy link
Contributor Author

The new results are finished and they are basically a wash: some slight improvements, some slight regressions. This makes no sense to me.

  • It doesn't match my local results at all.
  • My local results are consistent with the many ObligationForest optimizations I've done before: big wins for inflate and keccak, and smaller wins for cranelift-codegen, clap-rs, and serde.
  • shallow_resolve_unchanged is known to be very hot, and these commits clearly improve it; there are no trade-offs involved, the new code is simply better.

I don't know what to do with this.

@nnethercote
Optimize shallow_resolve_changed.
f5e68cc 
It can be made even more specialized.
@nnethercote
Change PendingPredicateObligation::stalled_on's type.
21f35bc 
From a `Vec<Ty>` to a `Vec<InferTy>`, because that's a more restrictive
type. This is a perf win because the ultra-hot function
`shallow_resolve_changed` has less pattern-matching to do.
@nnethercote
Copy link
Contributor Author

I did another local perf run, a full one this time. Results are the same as what I got before, and don't match the CI results:

inflate-check
        avg: -3.0%      min: -4.2%      max: 0.0%
keccak-check
        avg: -1.9%      min: -3.0%      max: -0.0%
keccak-debug
        avg: -1.7%      min: -2.6%      max: 0.0%
inflate-debug
        avg: -1.8%      min: -2.6%      max: -0.0%
keccak-opt
        avg: -1.5%      min: -2.3%      max: 0.0%
inflate-opt
        avg: -0.5%?     min: -0.7%?     max: 0.0%?
coercions-debug
        avg: -0.1%?     min: -0.5%?     max: 0.1%?
cranelift-codegen-check
        avg: -0.2%      min: -0.4%      max: 0.0%
clap-rs-check
        avg: -0.1%      min: -0.3%      max: -0.0%
serde-check
        avg: -0.2%      min: -0.3%      max: 0.0%
serde-debug
        avg: -0.2%      min: -0.3%      max: 0.0%
serde-opt
        avg: -0.2%      min: -0.3%      max: 0.0%

Argh.

@nnethercote nnethercote force-pushed the optimize-shallow_resolve_changed branch from 0cb6b45 to 21f35bc Compare December 11, 2019 12:34
@nnethercote
Copy link
Contributor Author

Let's try one more time.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 11, 2019

⌛ Trying commit 21f35bc with merge 9f4ea4a...

bors added a commit that referenced this pull request Dec 11, 2019
@bors
Auto merge of #67079 - nnethercote:optimize-shallow_resolve_changed, …
9f4ea4a 
…r=<try>

Optimize `shallow_resolve_changed`

r? @nikomatsakis
@Mark-Simulacrum
Copy link
Member

@nnethercote Maybe it's worth trying to benchmark locally with the CI build (i.e., using rustup-toolchain-install-master)? It seems not entirely unlikely that the difference comes down to incremental state or something like that.

@bors
Copy link
Contributor

bors commented Dec 11, 2019

☀️ Try build successful - checks-azure
Build commit: 9f4ea4a (9f4ea4a85c8473a279666626b279bf8a4ab0d15d)

@rust-timer
Copy link
Collaborator

Queued 9f4ea4a with parent 033662d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 9f4ea4a, comparison URL.

@nnethercote nnethercote mentioned this pull request Dec 11, 2019
Merged
@nikomatsakis
Copy link
Contributor

Having trouble with the comparison URL :( Not sure what's going on with those things

@nikomatsakis
Copy link
Contributor

Maybe it needs to be rebased so that the comparison is based off a more recent commit?

@nnethercote
Copy link
Contributor Author

The CI run does two sets of benchmarking, one for the commits, and one for the parent (the baseline). rust-timer reports back when the first set is done, but the URL doesn't display results successfully until the parent is also done. It's in the queue right now, should be finished soon.

I also suspect a bug in rustc-perf... I am also getting mismatches between local results and CI results in #66405. I am investigating now.

@nnethercote
Copy link
Contributor Author

The latest perf run has finished and the results are closer to what I'd expect (wins on inflate and keccak), but still don't fully match my local results. I'm about to investigate with local runs of the builds used for the CI run.

nnethercote added a commit to nnethercote/rust that referenced this pull request Dec 12, 2019
@nnethercote
[DO NOT LAND] Regress the token-stream-stress benchmark.
2d5843d 
I have a suspicion that there is a bug in rustc-perf or rust-timer
causing the wrong revisions to be measured by CI. See rust-lang#66405 and rust-lang#67079
for more details.

This commit deliberately causes a massive regression to the
`token-stream-stress` benchmark. On my machine, the instruction count
goes from 313M to 6084M, an 1843.4% regression. I want to see if a CI
run replicates that.
@nnethercote nnethercote mentioned this pull request Dec 12, 2019
Closed
bors added a commit that referenced this pull request Dec 12, 2019
@bors
Auto merge of #67248 - nnethercote:regress-token-stream-stress, r=<try>
5e1e02e 
[DO NOT LAND] Regress the `token-stream-stress` benchmark.

I have a suspicion that there is a bug in rustc-perf or rust-timer
causing the wrong revisions to be measured by CI. See #66405 and #67079
for more details.

This commit deliberately causes a massive regression to the
`token-stream-stress` benchmark. On my machine, the instruction count
goes from 313M to 6084M, an 1843.4% regression. I want to see if a CI
run replicates that.

cc @Mark-Simulacrum
r? @ghost
@nnethercote
Copy link
Contributor Author

#67248 suggests that CI is working as expected. The most recent CI perf results are good, even though they're not as good as what I saw locally. And logically, the code is clearly better. I will land this, though I'm still uneasy about the variability of the CI perf results.

r=nikomatsakis

@nnethercote
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 12, 2019

📌 Commit 21f35bc has been approved by nikomatsakis

@bors 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 Dec 12, 2019
@nnethercote
Copy link
Contributor Author

@bors rollup=never

@mati865
Copy link
Contributor

mati865 commented Dec 12, 2019 •
edited

@nnethercote are you building locally with jemalloc enabled? Also difference of glibc versions could make big difference here.

@bors
Copy link
Contributor

bors commented Dec 12, 2019

⌛ Testing commit 21f35bc with merge 3eebe05...

bors added a commit that referenced this pull request Dec 12, 2019
@bors
Auto merge of #67079 - nnethercote:optimize-shallow_resolve_changed, …
3eebe05 
…r=nikomatsakis

Optimize `shallow_resolve_changed`

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 12, 2019

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 3eebe05 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 12, 2019
@bors bors merged commit 21f35bc into rust-lang:master Dec 12, 2019
@nnethercote nnethercote deleted the optimize-shallow_resolve_changed branch December 12, 2019 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nnethercote @rust-timer @bors @nikomatsakis @Centril @Mark-Simulacrum @mati865 @rust-highfive