Make `unconditional_recursion` work across function boundaries #75067

jonas-schievink · 2020-08-02T18:46:33Z

First step towards #57965

rust-highfive · 2020-08-02T18:46:36Z

r? @oli-obk

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

jonas-schievink · 2020-08-02T18:46:45Z

@bors try @rust-timer queue

rust-timer · 2020-08-02T18:46:47Z

Awaiting bors try build completion

bors · 2020-08-02T18:46:57Z

⌛ Trying commit 6becae3d4de52eabf72d7b69f006573ddfecf267 with merge de45839e975f8517340a4e9a2c1cee0a92e4b812...

src/librustc_middle/ty/steal.rs

bors · 2020-08-02T19:26:24Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: de45839e975f8517340a4e9a2c1cee0a92e4b812 (de45839e975f8517340a4e9a2c1cee0a92e4b812)

rust-timer · 2020-08-02T19:26:26Z

Queued de45839e975f8517340a4e9a2c1cee0a92e4b812 with parent a99ae95, future comparison URL.

rust-timer · 2020-08-03T00:14:05Z

Finished benchmarking try commit (de45839e975f8517340a4e9a2c1cee0a92e4b812): 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

jonas-schievink · 2020-08-21T17:32:16Z

I've pushed a new commit that implements this in a better way, but for some reason it breaks all the incremental tests

lcnr

from tcx.ensure():

/// Ensure that either this query has all green inputs or been executed.

So if inevitable_calls is green and mir_const is not, we actually don't execute inevitable_calls. As we do not cache it on disk, this would mean that when we later need the actual query content, we do have to recompute it, but at this point mir_built has already been stolen afaict.

I think we can either cache this query on disk (which is probably quite costly) or not use ensure in mir_const 🤔

src/librustc_mir/lints.rs

jonas-schievink · 2020-08-22T14:12:24Z

Thanks, that fixed the issue!

jonas-schievink · 2020-08-22T14:13:11Z

Lets run perf again with the query in place.

@bors try @rust-timer queue

rust-timer · 2020-08-22T14:13:12Z

Awaiting bors try build completion

bors · 2020-08-22T14:13:22Z

⌛ Trying commit 95484733cd28cbf411a16c68f6a25cc1a8d38ed9 with merge 89143ca40dfdf0be97901f1853b15fe7a8e83a23...

bors · 2020-08-22T14:55:51Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 89143ca40dfdf0be97901f1853b15fe7a8e83a23 (89143ca40dfdf0be97901f1853b15fe7a8e83a23)

rust-timer · 2020-08-22T14:55:53Z

Queued 89143ca40dfdf0be97901f1853b15fe7a8e83a23 with parent 108e90c, future comparison URL.

rust-timer · 2020-08-22T16:16:27Z

Finished benchmarking try commit (89143ca40dfdf0be97901f1853b15fe7a8e83a23): 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

jonas-schievink · 2020-08-23T01:27:16Z

@bors try @rust-timer queue

rust-timer · 2020-08-23T01:27:18Z

Awaiting bors try build completion

bors · 2020-08-23T01:27:30Z

⌛ Trying commit f537570bae0605b2c6749ba9b16088720e2ec771 with merge d8aa519edb0359cf70d42b14771e587f3a6bc4ca...

bors · 2020-08-23T02:11:48Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: d8aa519edb0359cf70d42b14771e587f3a6bc4ca (d8aa519edb0359cf70d42b14771e587f3a6bc4ca)

rust-timer · 2020-08-23T02:11:50Z

Queued d8aa519edb0359cf70d42b14771e587f3a6bc4ca with parent 663d2f5, future comparison URL.

rust-timer · 2020-08-23T03:29:17Z

Finished benchmarking try commit (d8aa519edb0359cf70d42b14771e587f3a6bc4ca): 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

bjorn3 · 2020-08-23T07:59:07Z

Better than the previous perf run, but still a significant regression. (up to 8%)

lcnr · 2020-08-23T12:02:20Z

src/librustc_mir/lints.rs

+
+        // All callees that are guaranteed to be reached by every successor will also be reached by
+        // `bb`. Compute the intersection.
+        let successors = relevant_successors.copied();


nit: style

this copied seems somewhat weird, can we instead match on terminator.kind in the above match and directly iterate by value?

The SwitchInt targets is a Vec, otherwise this would work.

lcnr · 2020-08-23T12:17:20Z

src/librustc_mir/lints.rs

+) -> &'tcx [(DefId, SubstsRef<'tcx>, &'tcx [Span])] {
+    tcx.arena.alloc_from_iter(
+        find_inevitable_calls(tcx, key)
+            .map(|(callee, spans)| (callee.def_id, callee.substs, &*tcx.arena.alloc_slice(&spans))),


We do allocate a lot to have the correct spans here. I think it would make a lot of sense to do a perf run without storing the relevant spans and only mention the function header of used functions in the lint.

And if that reduces the perf impact using an ArrayVec and only storing a limited number of spans or using a SmallVec might be interesting.

lcnr · 2020-08-23T12:17:29Z

src/librustc_mir/lints.rs

+fn find_inevitable_calls<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    def_id: DefId,
+) -> impl Iterator<Item = (Callee<'tcx>, Vec<Span>)> {


Suggested change

) -> impl Iterator<Item = (Callee<'tcx>, Vec<Span>)> {

) -> Option<impl Iterator<Item = (Callee<'tcx>, Vec<Span>)>> {

nit: might be slightly more efficient and imo cleaner

jonas-schievink · 2020-08-23T13:57:32Z

@bors try @rust-timer queue

rust-timer · 2020-08-23T13:57:33Z

Awaiting bors try build completion

bors · 2020-08-23T13:57:43Z

⌛ Trying commit 94dc317f8b684b47db0af3c09f94bf11393cb63e with merge 55691358cfa29310103a6ec5c25e35f0f611abc6...

bors · 2020-08-23T14:42:36Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 55691358cfa29310103a6ec5c25e35f0f611abc6 (55691358cfa29310103a6ec5c25e35f0f611abc6)

rust-timer · 2020-08-23T14:42:37Z

Queued 55691358cfa29310103a6ec5c25e35f0f611abc6 with parent 2342cc3, future comparison URL.

rust-timer · 2020-08-23T15:59:19Z

Finished benchmarking try commit (55691358cfa29310103a6ec5c25e35f0f611abc6): 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

jonas-schievink · 2020-08-23T16:02:45Z

No changes from not tracking call spans. Most of the perf regressions seem to be due to more queries being run (but we shouldn't really call Instance::resolve more often than before). Oh, and inevitable_calls is still a bit slow.

jonas-schievink · 2020-08-23T19:11:33Z

@bors try @rust-timer queue

rust-timer · 2020-08-23T19:11:35Z

Awaiting bors try build completion

bors · 2020-08-23T19:11:40Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout recursion-lint (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self recursion-lint --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (modify/delete): src/librustc_mir_build/lints.rs deleted in heads/homu-tmp and modified in HEAD. Version HEAD of src/librustc_mir_build/lints.rs left in tree.
Auto-merging src/librustc_mir_build/build/mod.rs
Automatic merge failed; fix conflicts and then commit the result.

jonas-schievink · 2020-08-23T19:21:39Z

@bors try @rust-timer queue

rust-timer · 2020-08-23T19:21:40Z

Awaiting bors try build completion

bors · 2020-08-23T19:21:51Z

⌛ Trying commit d5d1603 with merge e8af76ec43b470394ef27523cf8644cf23cd741c...

bors · 2020-08-23T20:05:52Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: e8af76ec43b470394ef27523cf8644cf23cd741c (e8af76ec43b470394ef27523cf8644cf23cd741c)

rust-timer · 2020-08-23T20:05:54Z

Queued e8af76ec43b470394ef27523cf8644cf23cd741c with parent d02a209, future comparison URL.

rust-timer · 2020-08-23T21:48:56Z

Finished benchmarking try commit (e8af76ec43b470394ef27523cf8644cf23cd741c): 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

Dylan-DPC-zz · 2020-10-22T21:41:27Z

Closing this due to inactivity. The benchmarks don't paint a good picture as well.

rust-highfive assigned oli-obk Aug 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2020

lcnr reviewed Aug 2, 2020

View reviewed changes

src/librustc_middle/ty/steal.rs Show resolved Hide resolved

lcnr reviewed Aug 22, 2020

View reviewed changes

src/librustc_mir/lints.rs Outdated Show resolved Hide resolved

lcnr reviewed Aug 23, 2020

View reviewed changes

jonas-schievink added 8 commits August 23, 2020 21:12

Make unconditional_recursion work across functions

5ce4883

Address nits

3ef5ae5

Don't use ensure

59374d0

inevitable_calls: avoid costly HashSet ops

94d9d3f

Clean up iterator

7bf9442

Remove old lints module

1b278e8

Move propagation to its own function

d799f27

Don't collect calls we don't need

d5d1603

Dylan-DPC-zz closed this Oct 22, 2020

jonas-schievink mentioned this pull request Sep 22, 2021

Make the unconditional_recursion lint work across function calls #57965

Open

Nilstrieb mentioned this pull request Jun 26, 2023

Warn on circular code rust-lang/rust-clippy#11032

Open

	) -> impl Iterator<Item = (Callee<'tcx>, Vec<Span>)> {
	) -> Option<impl Iterator<Item = (Callee<'tcx>, Vec<Span>)>> {

Make unconditional_recursion work across function boundaries #75067

Make unconditional_recursion work across function boundaries #75067

Conversation

jonas-schievink commented Aug 2, 2020 • edited

rust-highfive commented Aug 2, 2020

jonas-schievink commented Aug 2, 2020

rust-timer commented Aug 2, 2020

bors commented Aug 2, 2020

bors commented Aug 2, 2020

rust-timer commented Aug 2, 2020

rust-timer commented Aug 3, 2020

jonas-schievink commented Aug 21, 2020

lcnr left a comment • edited

Choose a reason for hiding this comment

jonas-schievink commented Aug 22, 2020

jonas-schievink commented Aug 22, 2020

rust-timer commented Aug 22, 2020

bors commented Aug 22, 2020

bors commented Aug 22, 2020

rust-timer commented Aug 22, 2020

rust-timer commented Aug 22, 2020

jonas-schievink commented Aug 23, 2020

rust-timer commented Aug 23, 2020

bors commented Aug 23, 2020

bors commented Aug 23, 2020

rust-timer commented Aug 23, 2020

rust-timer commented Aug 23, 2020

bjorn3 commented Aug 23, 2020

lcnr Aug 23, 2020 • edited

Choose a reason for hiding this comment

jonas-schievink Aug 23, 2020

Choose a reason for hiding this comment

lcnr Aug 23, 2020

Choose a reason for hiding this comment

lcnr Aug 23, 2020

Choose a reason for hiding this comment

jonas-schievink commented Aug 23, 2020

rust-timer commented Aug 23, 2020

bors commented Aug 23, 2020

bors commented Aug 23, 2020

rust-timer commented Aug 23, 2020

rust-timer commented Aug 23, 2020

jonas-schievink commented Aug 23, 2020 • edited

jonas-schievink commented Aug 23, 2020

rust-timer commented Aug 23, 2020

bors commented Aug 23, 2020

jonas-schievink commented Aug 23, 2020

rust-timer commented Aug 23, 2020

bors commented Aug 23, 2020

bors commented Aug 23, 2020

rust-timer commented Aug 23, 2020

rust-timer commented Aug 23, 2020

Dylan-DPC-zz commented Oct 22, 2020

Make `unconditional_recursion` work across function boundaries #75067

Make `unconditional_recursion` work across function boundaries #75067

jonas-schievink commented Aug 2, 2020 •

edited

lcnr left a comment •

edited

lcnr Aug 23, 2020 •

edited

jonas-schievink commented Aug 23, 2020 •

edited