resolve: Eagerly feed closure visibilities #119136

petrochenkov · 2023-12-19T20:29:45Z

Also factor out all tcx-dependent operations performed for every created definition into TyCtxt::create_def.

Addresses #118657 (comment)

rustbot · 2023-12-19T20:29:54Z

r? @WaffleLapkin

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

petrochenkov · 2023-12-19T20:29:58Z

@bors try @rust-timer queue

bors · 2023-12-19T20:31:08Z

⌛ Trying commit 48e311f with merge 5b028d4...

resolve: Eagerly feed closure visibilities Addresses rust-lang#118657 (comment)

bors · 2023-12-19T21:57:22Z

☀️ Try build successful - checks-actions
Build commit: 5b028d4 (5b028d47cc3830b38690a943660a2912e4529c6d)

rust-timer · 2023-12-20T01:56:52Z

Finished benchmarking commit (5b028d4): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.5%	[-4.3%, -2.7%]	2
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	-1.6%	[-4.3%, 2.2%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 673.86s -> 672.275s (-0.24%)
Artifact size: 312.43 MiB -> 312.42 MiB (-0.00%)

Also factor out all tcx-dependent operations performed for every created definition into `TyCtxt::create_def`

petrochenkov · 2023-12-20T10:55:43Z

@rustbot ready
@bors rollup=maybe

WaffleLapkin

r=me after nits

WaffleLapkin · 2023-12-20T13:19:16Z

compiler/rustc_middle/src/ty/context.rs


-        let feed = TyCtxtFeed { tcx: self.tcx, key };
-        feed.def_kind(def_kind);
+        let feed = self.tcx.feed_local_def_id(def_id);
        feed.def_span(self.span);


Why is this span not simply passed to create_def?

This span is not available in resolver right now, it will be passed when the FIXME (#119136 (comment)) is addressed.

WaffleLapkin · 2023-12-20T13:21:48Z

compiler/rustc_resolve/src/lib.rs

@@ -1225,10 +1225,7 @@ impl<'tcx> Resolver<'_, 'tcx> {
        );

        // FIXME: remove `def_span` body, pass in the right spans here and call `tcx.at().create_def()`


Is this fixme up to date?

Yes, #118633 is trying to address it, but there are some complications.

petrochenkov · 2023-12-20T14:01:50Z

@bors r=WaffleLapkin

bors · 2023-12-20T14:01:52Z

📌 Commit 5e5d82e has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-12-20T14:03:03Z

⌛ Testing commit 5e5d82e with merge 8fca829...

bors · 2023-12-20T16:00:38Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 8fca829 to master...

rust-timer · 2023-12-20T18:33:45Z

Finished benchmarking commit (8fca829): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.6%, 0.8%]	2
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-1.8%	[-1.8%, -1.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.8%, 0.8%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.5%	[0.6%, 4.5%]	2
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[0.6%, 4.5%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.565s -> 674.047s (0.07%)
Artifact size: 312.83 MiB -> 312.81 MiB (-0.01%)

…=WaffleLapkin Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def` oversight from rust-lang#119136 Not actually an issue, because all uses of `tcx.create_def` were in the resolver, which is `eval_always`, but still good to harden against future uses of `create_def` cc `@petrochenkov` `@WaffleLapkin`

Rollup merge of rust-lang#121084 - oli-obk:create_def_forever_red2, r=WaffleLapkin Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def` oversight from rust-lang#119136 Not actually an issue, because all uses of `tcx.create_def` were in the resolver, which is `eval_always`, but still good to harden against future uses of `create_def` cc `@petrochenkov` `@WaffleLapkin`

…=WaffleLapkin Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def` oversight from rust-lang#119136 Not actually an issue, because all uses of `tcx.create_def` were in the resolver, which is `eval_always`, but still good to harden against future uses of `create_def` cc `@petrochenkov` `@WaffleLapkin`

Rollup merge of rust-lang#121084 - oli-obk:create_def_forever_red2, r=WaffleLapkin Make sure `tcx.create_def` also depends on the forever red node, instead of just `tcx.at(span).create_def` oversight from rust-lang#119136 Not actually an issue, because all uses of `tcx.create_def` were in the resolver, which is `eval_always`, but still good to harden against future uses of `create_def` cc `@petrochenkov` `@WaffleLapkin`

rustbot assigned WaffleLapkin Dec 19, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 19, 2023

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 19, 2023

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2023

Auto merge of rust-lang#119136 - petrochenkov:feedvis3, r=<try>

5b028d4

resolve: Eagerly feed closure visibilities Addresses rust-lang#118657 (comment)

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 20, 2023

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 20, 2023

resolve: Eagerly feed closure visibilities

5e5d82e

Also factor out all tcx-dependent operations performed for every created definition into `TyCtxt::create_def`

petrochenkov force-pushed the feedvis3 branch from 48e311f to 5e5d82e Compare December 20, 2023 10:53

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2023

WaffleLapkin approved these changes Dec 20, 2023

View reviewed changes

WaffleLapkin 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2023

bors merged commit 8fca829 into rust-lang:master Dec 20, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Dec 20, 2023

oli-obk mentioned this pull request Feb 14, 2024

Make sure tcx.create_def also depends on the forever red node, instead of just tcx.at(span).create_def #121084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: Eagerly feed closure visibilities #119136

resolve: Eagerly feed closure visibilities #119136

petrochenkov commented Dec 19, 2023 •

edited

Loading

rustbot commented Dec 19, 2023

petrochenkov commented Dec 19, 2023

This comment has been minimized.

bors commented Dec 19, 2023

bors commented Dec 19, 2023

This comment has been minimized.

rust-timer commented Dec 20, 2023

petrochenkov commented Dec 20, 2023

WaffleLapkin left a comment

WaffleLapkin Dec 20, 2023

petrochenkov Dec 20, 2023

WaffleLapkin Dec 20, 2023

petrochenkov Dec 20, 2023

petrochenkov commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

rust-timer commented Dec 20, 2023

		@@ -1225,10 +1225,7 @@ impl<'tcx> Resolver<'_, 'tcx> {
		);

		// FIXME: remove `def_span` body, pass in the right spans here and call `tcx.at().create_def()`

resolve: Eagerly feed closure visibilities #119136

resolve: Eagerly feed closure visibilities #119136

Conversation

petrochenkov commented Dec 19, 2023 • edited Loading

rustbot commented Dec 19, 2023

petrochenkov commented Dec 19, 2023

This comment has been minimized.

bors commented Dec 19, 2023

bors commented Dec 19, 2023

This comment has been minimized.

rust-timer commented Dec 20, 2023

Overall result: no relevant changes - no action needed

petrochenkov commented Dec 20, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

bors commented Dec 20, 2023

rust-timer commented Dec 20, 2023

Overall result: ❌✅ regressions and improvements - no action needed

petrochenkov commented Dec 19, 2023 •

edited

Loading