resolve: Stop creating `NameBinding`s on every use, create them once per definition instead #113408

petrochenkov · 2023-07-06T14:03:32Z

NameBinding values are supposed to be unique, use referential equality, and be created once for every name declaration.

Before this PR many NameBindings were created on name use, rather than on name declaration, because it's sufficiently cheap, and comparisons are not actually used in practice for some binding kinds.
This PR makes NameBindings consistently unique and created on name declaration.

There are two special cases

for extern prelude names creating NameBinding requires loading the corresponding crate, which is expensive, so such bindings are created lazily on first use, but they still keep the uniqueness by being reused on further uses.
for legacy derive helpers (helper attributes written before derives that introduce them) the declaration and the use is basically the same thing (that's one of the reasons why they are deprecated), so they are still created on use, but we can still maybe do a bit better in a way that I described in FIXME in the last commit.

rustbot · 2023-07-06T14:03:41Z

r? @fee1-dead

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

petrochenkov · 2023-07-06T14:04:06Z

@bors try @rust-timer queue

bors · 2023-07-06T14:04:19Z

⌛ Trying commit b8cb9b3f49d53db6efd3613b0a8a37bbe8b82642 with merge fb3c2f2e93d4207348fe7c62386f58b91dc6596f...

bors · 2023-07-06T15:21:42Z

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

bors · 2023-07-06T15:21:42Z

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

rust-timer · 2023-07-06T16:38:12Z

Finished benchmarking commit (fb3c2f2e93d4207348fe7c62386f58b91dc6596f): comparison URL.

Overall result: ❌ regressions - 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 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.7%	[0.6%, 0.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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
Regressions ❌ (secondary)	6.3%	[3.8%, 8.8%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.5%, -2.2%]	3
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 656.625s -> 657.843s (0.19%)

fee1-dead · 2023-08-22T08:16:51Z

cc #115089

r? compiler

petrochenkov · 2023-08-23T07:53:54Z

@bors try @rust-timer queue

bors · 2023-08-23T07:54:03Z

⌛ Trying commit 53c72df6b05587af5b44d9019fef25c49f15009d with merge eebbf243e083fba445d50a681206762964d9be19...

bors · 2023-08-23T09:07:02Z

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

rust-timer · 2023-08-23T11:19:08Z

Finished benchmarking commit (eebbf243e083fba445d50a681206762964d9be19): comparison URL.

Overall result: ❌ regressions - 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 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.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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
Regressions ❌ (secondary)	2.8%	[1.3%, 4.8%]	7
Improvements ✅ (primary)	-1.0%	[-1.3%, -0.7%]	2
Improvements ✅ (secondary)	-4.1%	[-9.5%, -1.9%]	7
All ❌✅ (primary)	-1.0%	[-1.3%, -0.7%]	2

Cycles

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

Binary size

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

Bootstrap: 634.141s -> 634.262s (0.02%)
Artifact size: 347.08 MiB -> 347.09 MiB (0.00%)

petrochenkov · 2023-08-23T11:22:16Z

@bors rollup=maybe
@rustbot ready

cjgillot · 2023-08-23T16:50:09Z

compiler/rustc_resolve/src/lib.rs

            } else {
+                update_binding = true;


Could you add a comment explaining what happens here?

Well, it's a micro-optimization to avoid resetting binding to the same value when it already exists, but it's likely useless so I'll just remove it.

cjgillot · 2023-08-23T16:53:01Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+                .extern_prelude
+                .entry(ident.normalize_to_macros_2_0())
+                .or_insert(ExternPreludeEntry { binding: None, introduced_by_item: true });
+            entry.binding = Some(imported_binding);


What happens if binding is already Some?

This is a case where binding from command line (--extern foo) is replaced with a binding from extern crate foo item in source code.

cjgillot · 2023-08-23T16:55:56Z

compiler/rustc_resolve/src/lib.rs

@@ -1006,6 +1006,7 @@ pub struct Resolver<'a, 'tcx> {
    dummy_binding: NameBinding<'a>,
    builtin_types_bindings: FxHashMap<Symbol, NameBinding<'a>>,
    builtin_attrs_bindings: FxHashMap<Symbol, NameBinding<'a>>,
+    crate_root_bindings: FxHashMap<Module<'a>, NameBinding<'a>>,


Could you add a comment for this field? Which name does NameBinding correspond to?

The name crate (or $crate), which is not explicitly declared anywhere.

Maybe it makes sense to route self for a module through the same binding as well, but self currently never produces NameBindings due to being processed through a special case in resolve_path_with_ribs (would be nice to make it more uniform).

instead of creating a new every time a name from extern prelude is accessed

instead of creating a new every time `crate` or `$crate` is used

instead of creating them every time such attribute is used

petrochenkov · 2023-08-24T11:53:41Z

Updated.

cjgillot · 2023-08-24T16:16:21Z

Thanks!
@bors r+

bors · 2023-08-24T16:16:23Z

📌 Commit 453edeb has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-08-24T20:06:00Z

⌛ Testing commit 453edeb with merge 58eefc3...

bors · 2023-08-24T21:53:08Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 58eefc3 to master...

rust-timer · 2023-08-25T02:02:16Z

Finished benchmarking commit (58eefc3): comparison URL.

Overall result: ❌ regressions - 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)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.5%	[1.4%, 4.0%]	3
Regressions ❌ (secondary)	2.5%	[0.9%, 4.5%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.6%	[-7.4%, -3.2%]	4
All ❌✅ (primary)	2.5%	[1.4%, 4.0%]	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)	1.2%	[1.2%, 1.2%]	2
Regressions ❌ (secondary)	2.7%	[2.5%, 2.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	2

Binary size

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

Bootstrap: 630.278s -> 629.602s (-0.11%)
Artifact size: 315.65 MiB -> 315.61 MiB (-0.01%)

rustbot assigned fee1-dead Jul 6, 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 Jul 6, 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 Jul 6, 2023

petrochenkov 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 Jul 6, 2023

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 Jul 6, 2023

petrochenkov force-pushed the bindintern2 branch from b8cb9b3 to cadc7be Compare August 18, 2023 09:29

petrochenkov changed the title ~~[WIP] resolve: Pre-intern builtin name bindings~~ [WIP] resolve: Stop creating NameBindings on every use, create them once per definition instead Aug 21, 2023

rustbot assigned cjgillot and unassigned fee1-dead Aug 22, 2023

petrochenkov force-pushed the bindintern2 branch 2 times, most recently from d38ba0e to 53c72df Compare August 23, 2023 07:51

petrochenkov changed the title ~~[WIP] resolve: Stop creating NameBindings on every use, create them once per definition instead~~ resolve: Stop creating NameBindings on every use, create them once per definition instead Aug 23, 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 Aug 23, 2023

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 Aug 23, 2023

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 Aug 23, 2023

cjgillot reviewed Aug 23, 2023

View reviewed changes

petrochenkov added 6 commits August 24, 2023 18:11

resolve: Pre-intern builtin name bindings

9ce3519

resolve: Make bindings from extern prelude unique

05010b6

instead of creating a new every time a name from extern prelude is accessed

resolve: Pre-intern tool module bindings

d1f8ea4

resolve: Make bindings for crate roots unique

02640f9

instead of creating a new every time `crate` or `$crate` is used

resolve: Make bindings for derive helper attributes unique

50bbe01

instead of creating them every time such attribute is used

resolve: Leave a comment about name bindings for legacy derive helpers

453edeb

petrochenkov force-pushed the bindintern2 branch from 53c72df to 453edeb Compare August 24, 2023 11:52

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 Aug 24, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2023

bors merged commit 58eefc3 into rust-lang:master Aug 24, 2023
12 checks passed

rustbot added this to the 1.74.0 milestone Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve: Stop creating `NameBinding`s on every use, create them once per definition instead #113408

resolve: Stop creating `NameBinding`s on every use, create them once per definition instead #113408

petrochenkov commented Jul 6, 2023 •

edited

rustbot commented Jul 6, 2023

petrochenkov commented Jul 6, 2023

This comment has been minimized.

bors commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

This comment has been minimized.

rust-timer commented Jul 6, 2023

fee1-dead commented Aug 22, 2023 •

edited

petrochenkov commented Aug 23, 2023

This comment has been minimized.

bors commented Aug 23, 2023

bors commented Aug 23, 2023

This comment has been minimized.

rust-timer commented Aug 23, 2023

petrochenkov commented Aug 23, 2023

cjgillot Aug 23, 2023

petrochenkov Aug 24, 2023

cjgillot Aug 23, 2023

petrochenkov Aug 24, 2023

cjgillot Aug 23, 2023

petrochenkov Aug 24, 2023

petrochenkov commented Aug 24, 2023

cjgillot commented Aug 24, 2023

bors commented Aug 24, 2023

bors commented Aug 24, 2023

bors commented Aug 24, 2023

rust-timer commented Aug 25, 2023

resolve: Stop creating NameBindings on every use, create them once per definition instead #113408

resolve: Stop creating NameBindings on every use, create them once per definition instead #113408

Conversation

petrochenkov commented Jul 6, 2023 • edited

rustbot commented Jul 6, 2023

petrochenkov commented Jul 6, 2023

This comment has been minimized.

bors commented Jul 6, 2023

bors commented Jul 6, 2023

bors commented Jul 6, 2023

This comment has been minimized.

rust-timer commented Jul 6, 2023

Overall result: ❌ regressions - no action needed

fee1-dead commented Aug 22, 2023 • edited

petrochenkov commented Aug 23, 2023

This comment has been minimized.

bors commented Aug 23, 2023

bors commented Aug 23, 2023

This comment has been minimized.

rust-timer commented Aug 23, 2023

Overall result: ❌ regressions - no action needed

petrochenkov commented Aug 23, 2023

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

Choose a reason for hiding this comment

petrochenkov commented Aug 24, 2023

cjgillot commented Aug 24, 2023

bors commented Aug 24, 2023

bors commented Aug 24, 2023

bors commented Aug 24, 2023

rust-timer commented Aug 25, 2023

Overall result: ❌ regressions - no action needed

resolve: Stop creating `NameBinding`s on every use, create them once per definition instead #113408

resolve: Stop creating `NameBinding`s on every use, create them once per definition instead #113408

petrochenkov commented Jul 6, 2023 •

edited

fee1-dead commented Aug 22, 2023 •

edited