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

perf: Only process changed obligations in ObligationForest #69218

Closed
wants to merge 25 commits into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Feb 16, 2020

This rewrites most of ObligationForest forest so to avoid iterating
through the entire set of obligations on each select attempt. Instead
only the obligations that can actually make progress are processed. This
gives great speedups in benchmarks such as inflate which create a
large number of pending obligations which fail to make progress.

To support this the unification tables were extended to keep track
of which type inference variables that has actually changed at each
step. Which then lets ObligationForest get a list of only the changed
variables at each step which it can map back to its obligations.

In addition to this primary change, many of the other iterations in
ObligationForest were refactored to only process lists of the nodes
that they actually are interested in. The extra bookkeeping needed for
this was possible without the primary change but were a performance
regressions there as they slowed down the main loop. As the main loop is
no longer the main issue these optimizations could be re-applied.

This needs some unit tests to the added data structures (I have some in an external crate I experimented in which I need to pull in), the warn! debug logs need to go/made debug! and I am not happy with the how Offsets are deregistered.

I would interested in getting some feedback on the overall PR and seeing some perf results though (the extra complexity to in unification could see some regressions, there may ways to reduce the added overhead there further).

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 16, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 16, 2020

⌛ Trying commit 7f32c87ad1fb9dd11733035bf98a8614748ee282 with merge a495eb849d6fa99996c180ca6e3012f402970a96...

@bors
Copy link
Contributor

bors commented Feb 17, 2020

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

@rust-timer
Copy link
Collaborator

Queued a495eb849d6fa99996c180ca6e3012f402970a96 with parent 5e7af46, future comparison URL.

@Marwes Marwes force-pushed the fulfill_next branch 2 times, most recently from 3970fea to 98a1b65 Compare February 17, 2020 13:37
@eddyb
Copy link
Member

eddyb commented Feb 18, 2020

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Feb 18, 2020
@Marwes
Copy link
Contributor Author

Marwes commented Feb 18, 2020

Shrank the snapshotting overhead quite a bit which removed most of the slowdown in wf-projection-stress. Another perf run to see if it was enough for the overall slowdown as well would be nice!

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 18, 2020

⌛ Trying commit 4b13fa94c89d243fa1c0148610996eaaf84ba955 with merge e3b5470b757aa1e38c8fb2feecc31966ce79513c...

@bors
Copy link
Contributor

bors commented Feb 18, 2020

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

@rust-timer
Copy link
Collaborator

Queued e3b5470b757aa1e38c8fb2feecc31966ce79513c with parent b0d5813, future comparison URL.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-20T18:41:19.0882261Z ========================== Starting Command Output ===========================
2020-02-20T18:41:19.0886002Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/588fe109-9643-4280-ae1b-c64984ed0174.sh
2020-02-20T18:41:19.0886462Z 
2020-02-20T18:41:19.0890175Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-20T18:41:19.0910105Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:41:19.0913472Z Task         : Get sources
2020-02-20T18:41:19.0913827Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-20T18:41:19.0914143Z Version      : 1.0.0
2020-02-20T18:41:19.0914358Z Author       : Microsoft
---
2020-02-20T18:41:20.0811700Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-20T18:41:20.0817838Z ##[command]git config gc.auto 0
2020-02-20T18:41:20.0821556Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-20T18:41:20.0825026Z ##[command]git config --get-all http.proxy
2020-02-20T18:41:20.0831093Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69218/merge:refs/remotes/pull/69218/merge
---
2020-02-20T18:45:11.6385837Z 
2020-02-20T18:45:11.6625275Z ##################################################                        70.0%
2020-02-20T18:45:11.6625824Z ######################################################################## 100.0%
2020-02-20T18:45:11.9527970Z extracting /checkout/obj/build/cache/2020-01-31/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.gz
2020-02-20T18:45:11.9827310Z error: failed to read `/ena/Cargo.toml`
2020-02-20T18:45:11.9827951Z Caused by:
2020-02-20T18:45:11.9828381Z   No such file or directory (os error 2)
2020-02-20T18:45:11.9837646Z failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml
2020-02-20T18:45:11.9838169Z Build completed unsuccessfully in 0:00:13
2020-02-20T18:45:11.9838169Z Build completed unsuccessfully in 0:00:13
2020-02-20T18:45:11.9886425Z == clock drift check ==
2020-02-20T18:45:11.9897341Z   local time: Thu Feb 20 18:45:11 UTC 2020
2020-02-20T18:45:12.2742909Z   network time: Thu, 20 Feb 2020 18:45:12 GMT
2020-02-20T18:45:12.2747505Z == end clock drift check ==
2020-02-20T18:45:19.8183803Z 
2020-02-20T18:45:19.8252488Z ##[error]Bash exited with code '1'.
2020-02-20T18:45:19.8277279Z ##[section]Finishing: Run build
2020-02-20T18:45:19.8327638Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:45:19.8332827Z Task         : Get sources
2020-02-20T18:45:19.8333204Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-20T18:45:19.8333566Z Version      : 1.0.0
2020-02-20T18:45:19.8333834Z Author       : Microsoft
2020-02-20T18:45:19.8333834Z Author       : Microsoft
2020-02-20T18:45:19.8334239Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-20T18:45:19.8334710Z ==============================================================================
2020-02-20T18:45:20.1622063Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-20T18:45:20.1672168Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:45:20.1759979Z Cleaning up task key
2020-02-20T18:45:20.1761347Z Start cleaning up orphan processes.
2020-02-20T18:45:20.1930030Z Terminate orphan process: pid (4271) (python)
2020-02-20T18:45:20.2095921Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-20T18:48:16.6491317Z ========================== Starting Command Output ===========================
2020-02-20T18:48:16.6493419Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/609c4a98-3cc6-42e9-be19-76b840d5196e.sh
2020-02-20T18:48:16.6493612Z 
2020-02-20T18:48:16.6496069Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-20T18:48:16.6511030Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:48:16.6513721Z Task         : Get sources
2020-02-20T18:48:16.6513984Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-20T18:48:16.6514192Z Version      : 1.0.0
2020-02-20T18:48:16.6514335Z Author       : Microsoft
---
2020-02-20T18:48:17.9854969Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-20T18:48:17.9860925Z ##[command]git config gc.auto 0
2020-02-20T18:48:17.9863931Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-20T18:48:17.9866773Z ##[command]git config --get-all http.proxy
2020-02-20T18:48:17.9873112Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69218/merge:refs/remotes/pull/69218/merge
---
2020-02-20T18:50:54.9237258Z #                                                                          2.1%
2020-02-20T18:50:54.9237656Z ######################################################################## 100.0%
2020-02-20T18:50:55.1277591Z extracting /checkout/obj/build/cache/2020-01-31/rustfmt-nightly-x86_64-unknown-linux-gnu.tar.gz
2020-02-20T18:50:55.1913844Z     Updating crates.io index
2020-02-20T18:51:10.6792805Z     Updating git repository `https://github.com/Marwes/ena`
---
2020-02-20T18:54:05.3531544Z tidy check
2020-02-20T18:54:06.5060214Z * 589 error codes
2020-02-20T18:54:06.5060505Z * highest error code: E0746
2020-02-20T18:54:06.7889448Z * 281 features
2020-02-20T18:54:07.2947622Z invalid source: "git+https://github.com/Marwes/ena?branch=detach_undo_log#9b977ea7f209a35f46d65d33cdd74b8f4931fb8a"
2020-02-20T18:54:07.5196544Z some tidy checks failed
2020-02-20T18:54:07.5196773Z Found 487 error codes
2020-02-20T18:54:07.5197002Z Found 0 error codes with no tests
2020-02-20T18:54:07.5197153Z Done!
2020-02-20T18:54:07.5197153Z Done!
2020-02-20T18:54:07.5207436Z 
2020-02-20T18:54:07.5207569Z 
2020-02-20T18:54:07.5213755Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-02-20T18:54:07.5214827Z 
2020-02-20T18:54:07.5219995Z 
2020-02-20T18:54:07.5220420Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-02-20T18:54:07.5220933Z Build completed unsuccessfully in 0:01:24
2020-02-20T18:54:07.5220933Z Build completed unsuccessfully in 0:01:24
2020-02-20T18:54:07.5264933Z == clock drift check ==
2020-02-20T18:54:07.5278530Z   local time: Thu Feb 20 18:54:07 UTC 2020
2020-02-20T18:54:07.6924965Z   network time: Thu, 20 Feb 2020 18:54:07 GMT
2020-02-20T18:54:07.6930060Z == end clock drift check ==
2020-02-20T18:54:08.5451181Z 
2020-02-20T18:54:08.5523660Z ##[error]Bash exited with code '1'.
2020-02-20T18:54:08.5534885Z ##[section]Finishing: Run build
2020-02-20T18:54:08.5606936Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:54:08.5614599Z Task         : Get sources
2020-02-20T18:54:08.5614914Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-20T18:54:08.5615456Z Version      : 1.0.0
2020-02-20T18:54:08.5615813Z Author       : Microsoft
2020-02-20T18:54:08.5615813Z Author       : Microsoft
2020-02-20T18:54:08.5616481Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-20T18:54:08.5617159Z ==============================================================================
2020-02-20T18:54:08.8234109Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-20T18:54:08.8273480Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69218/merge to s
2020-02-20T18:54:08.8351439Z Cleaning up task key
2020-02-20T18:54:08.8352504Z Start cleaning up orphan processes.
2020-02-20T18:54:08.8497565Z Terminate orphan process: pid (4520) (python)
2020-02-20T18:54:08.8625191Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Marwes
Copy link
Contributor Author

Marwes commented Feb 21, 2020

Pushed a commit which pulls in some modifications to https://github.com/rust-lang-nursery/ena as a git dependency. Disablde the tidy check for git dependencies to get it to build for a perf run.

With the modification to ena it was possible to reduce the snapshot overhead almost entirely which at least seems to fix the regression in wf-projection-stress locally. The other regressions seems to be for some other reason than snapshotting which I have yet to determine.

If the ena change to support an separately passed undo_log is good then the same change could be used to further reduce snapshot overhead by letting all types that are snapshotted share the same log.

Another perf run would be nice in any case.

@Marwes
Copy link
Contributor Author

Marwes commented Feb 21, 2020

(I have held of on documenting and cleaning this up so far as much is still in flux while I try to fix the regressions).

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 21, 2020

⌛ Trying commit 56f916fab9aa2d644f3f11cf50ac282a7fa530e4 with merge 2c67e37fa0d8c84ac85061c83cf8a39ceeb24abd...

@bors
Copy link
Contributor

bors commented Oct 26, 2020

☀️ Try build successful - checks-actions
Build commit: 30a3e111a6443337457980ccf94a22d0b8c9d02e (30a3e111a6443337457980ccf94a22d0b8c9d02e)

@rust-timer
Copy link
Collaborator

Queued 30a3e111a6443337457980ccf94a22d0b8c9d02e with parent 9f6c670, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (30a3e111a6443337457980ccf94a22d0b8c9d02e): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Diggsey
Copy link
Contributor

Diggsey commented Oct 26, 2020

Interesting, there does appear to be more green, but probably still too much red overall...

@Marwes
Copy link
Contributor Author

Marwes commented Oct 26, 2020

Added a commit to always use the simple, current obligation forest processing. A perf run should show how much the complexity added to the obligation forest slows things down.

@nikomatsakis
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 27, 2020

⌛ Trying commit d3d3fc4 with merge 77eafb1583b505bea6e1537365a7ae77bc3febf4...

@bors
Copy link
Contributor

bors commented Oct 27, 2020

☀️ Try build successful - checks-actions
Build commit: 77eafb1583b505bea6e1537365a7ae77bc3febf4 (77eafb1583b505bea6e1537365a7ae77bc3febf4)

@rust-timer
Copy link
Collaborator

Queued 77eafb1583b505bea6e1537365a7ae77bc3febf4 with parent 20b1e05, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (77eafb1583b505bea6e1537365a7ae77bc3febf4): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Marwes
Copy link
Contributor Author

Marwes commented Oct 27, 2020

https://perf.rust-lang.org/compare.html?start=77eafb1583b505bea6e1537365a7ae77bc3febf4&end=30a3e111a6443337457980ccf94a22d0b8c9d02e&stat=instructions%3Au

Is a comparison of the current version which does not use the precise tracking at all, it uses the same way to work through the obligationforest. All benchmark excepts one are improved by the precise processing so it does not appear as if the obligation forest is the culprit for the slow down.

The added LoggedUnificationTable does still need to check if it needs to track a variable when it unifies so I suspect it is these extra checks doing the slowdown. I removed the code responsible for this so if I am correct a new perf run should show no or neglible slowdown.

(Sorry for the spam, I'd run this locally but I can't seem to get reliable results for such relatively small changes).

@Mark-Simulacrum
Copy link
Member

That doesn't look like a proper comparison - the parent commits on master for those two are different. If you want to re-run the perf builds we can do so, give me a ping on Zulip and I should be able to help out.

@Marwes
Copy link
Contributor Author

Marwes commented Oct 28, 2020

Since perf runs comparions on merge commits I don't think there is a way to have have proper parents for perf runs on try builds? It only matters if there are significant performance changes in merged in between though (I am only looking for a rough comparison here).

@Mark-Simulacrum
Copy link
Member

There's two strategies:

  • Kick off near-simultaneous try builds such that the parent commit happens to be the same
  • For the second try build, push reverts of all commits on master up to the parent you need, and make sure that the try build kicks off with the right parent commit (just so happens to be)

@bors
Copy link
Contributor

bors commented Nov 5, 2020

☔ The latest upstream changes (presumably #78767) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2020
@JohnCSimon
Copy link
Member

Ping from triage -
@Marwes
Can you please post your status on this PR? It hasn't seen any movement in a month. Thanks.

@Marwes Marwes closed this Nov 24, 2020
@jyn514
Copy link
Member

jyn514 commented Jan 24, 2021

I'm not sure why this was closed :/ the perf results look really good, I'd love to have this improvement.

@Marwes
Copy link
Contributor Author

Marwes commented Jan 24, 2021

It only improves things in a few specific instances otherwise it is a a regression due to the increased cost of the added data structures https://perf.rust-lang.org/compare.html?start=9f6c670c4b4273b2c7c0af07a524d4240c926bfc&end=30a3e111a6443337457980ccf94a22d0b8c9d02e . I have fixed some of that locally but I don't believe it makes up enough of the regression so for the moment this isn't an improvement.

@jyn514
Copy link
Member

jyn514 commented Jan 24, 2021

Personally I think -50% on inflate is absolutely worth it, the regressions are much more minor in comparison, and the doc times are pretty variable anyway.

@Marwes
Copy link
Contributor Author

Marwes commented Jan 24, 2021

The wins are on an old version of inflate which heavily abuses macros instead of inline functions, causing an absolutely enormous function to be generated. So I would expect wins like that to be exceedingly rare in the average crate, the regressions on the other hand affects almost every crate so overall it would definitely be a loss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. relnotes-perf Performance improvements that should be mentioned in the release notes. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.