Split create_def and lowering of lifetimes for opaque types and bare async fns #99867

spastorino · 2022-07-28T18:12:45Z

This work is kind of half-way, but I think it could be merged anyway.
I think we should be able to remove all the vacant arms in new_named_lifetime_with_res, if I'm not wrong that requires visiting more nodes. We can do that as a follow up.
In follow-up PRs, besides the thing mentioned previously, I'll be trying to remove LifetimeCaptureContext, captured_lifetimes as a global data structure, global binders_to_ignore and all their friends :).

Also try to remap in a more general way based on def-ids.

oli-obk · 2022-07-29T08:21:29Z

@bors try @rust-timer queue

rust-timer · 2022-07-29T08:21:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-29T08:21:37Z

⌛ Trying commit 7ad47ea4f6cb9c52947e3412e5bf9ab023375d59 with merge c43f4be8df1097e58deddd266b2f364a3d03dc0a...

bors · 2022-07-29T09:50:03Z

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

rust-timer · 2022-07-29T09:50:05Z

Queued c43f4be8df1097e58deddd266b2f364a3d03dc0a with parent ea6ab1b, future comparison URL.

rust-timer · 2022-07-29T13:29:35Z

Finished benchmarking commit (c43f4be8df1097e58deddd266b2f364a3d03dc0a): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.7%	4.5%	2
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Cycles

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-2.5%	-2.5%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.5%	-2.5%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

cjgillot · 2022-07-31T18:51:33Z

Thanks @spastorino. I'm not sure I understand what this PR does.
From what I gather: this PR mostly collects some of the lifetimes that appear in the return type, and eagerly inserts them into the captures.

why not collect all the lifetimes straight away?
why not filter HRTB during this collection, instead of later?
is there a way to remove those Vacant arms in lower_lifetime_with_res in this PR?

spastorino · 2022-08-01T02:06:22Z

@cjgillot thanks for the review. First of all, agreed with all your questions and as I've said, this would be the first part of a set of changes that I'd like to land that go in the direction you're saying.

why not collect all the lifetimes straight away?

Yes, agreed and this is what I wanted to discuss with you. Maybe if you have some time we should schedule some sync time on zulip?.

why not filter HRTB during this collection, instead of later?

Same to this.

is there a way to remove those Vacant arms in lower_lifetime_with_res in this PR?

This is the same thing I wonder and I'd like to do. Again if we can discuss this in a synchronous way would be better I believe.

compiler/rustc_ast/src/ast.rs

compiler/rustc_ast_lowering/src/lib.rs

compiler/rustc_ast_lowering/src/lifetime_collector.rs

spastorino · 2022-08-02T00:44:19Z

@cjgillot @nikomatsakis have pushed some changes, please check them out.
Anyway this is not working due to the way I'm handling Elided on Rptr, in particular https://github.com/rust-lang/rust/pull/99867/files#diff-ad0c15bbde97a607d4758ec7eaf88248be5d6b8ae084dfc84127f81e3f7a9bb4R1488, unsure if that's using the right parent def_id. Any tip would be appreciated.

compiler/rustc_ast_lowering/src/lib.rs

nikomatsakis · 2022-08-02T11:05:39Z

compiler/rustc_ast_lowering/src/lib.rs

+            // Future<Output = &'1 [ &'2 u32 ]>`.
+            //
+            // Then, we will create `fn foo(..) -> Foo<'_, '_>`, and
+            // hence the elision takes place at the fn site.


This comment is likely out of date, right? Elision is happening in an earlier phase now, I think.

cc @cjgillot

nikomatsakis

A few doc nits, looks good though!

compiler/rustc_ast_lowering/src/lib.rs

nikomatsakis · 2022-08-04T15:58:05Z

@bors r+

Nice work.

bors · 2022-08-04T15:58:07Z

📌 Commit fb7e44dd22c886a0d76c9223df9735cae76b385e has been approved by nikomatsakis

It is now in the queue for this repository.

spastorino · 2022-08-04T18:14:27Z

@bors r=nikomatsakis

bors · 2022-08-04T18:14:29Z

📌 Commit 4170d73 has been approved by nikomatsakis

It is now in the queue for this repository.

bors · 2022-08-05T06:35:16Z

⌛ Testing commit 4170d73 with merge cdfd675...

bors · 2022-08-05T09:32:19Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing cdfd675 to master...

rust-timer · 2022-08-05T10:48:59Z

Finished benchmarking commit (cdfd675): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.3%	1.4%	6
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.2%	-1.2%	1
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

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

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	4.0%	4.0%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rust-highfive assigned cjgillot Jul 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2022

spastorino force-pushed the refactor-remap-lifetimes branch 6 times, most recently from 2e84851 to 7ad47ea Compare July 28, 2022 23:47

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 29, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 29, 2022

nikomatsakis reviewed Aug 1, 2022

View reviewed changes

spastorino force-pushed the refactor-remap-lifetimes branch from 584a538 to 7915f15 Compare August 1, 2022 19:14

cjgillot reviewed Aug 1, 2022

View reviewed changes

compiler/rustc_ast_lowering/src/lifetime_collector.rs Show resolved Hide resolved

cjgillot reviewed Aug 1, 2022

View reviewed changes

compiler/rustc_ast_lowering/src/lifetime_collector.rs Outdated Show resolved Hide resolved

spastorino force-pushed the refactor-remap-lifetimes branch 2 times, most recently from 2493ab6 to 6d79822 Compare August 2, 2022 00:41

This comment has been minimized.

Sign in to view

nikomatsakis reviewed Aug 2, 2022

View reviewed changes

spastorino force-pushed the refactor-remap-lifetimes branch from 6d79822 to cbf7fea Compare August 2, 2022 14:03

spastorino added 8 commits August 4, 2022 11:27

Add documentation about lifetime args

cab6740

Add more debug calls

2f353d1

Add documentation for create_lifetime_defs

1ece866

Move new_remapping inside with_hir_id_owner

9f10f58

Extract record_elided_anchor

f8b1b2b

Add docs to record_elided_anchor

5e71659

Add docs to generics_def_id_map

c0923c8

Use span_bug instead of panic

45991f9

nikomatsakis requested changes Aug 4, 2022

View reviewed changes

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

Do not collect lifetimes with Infer resolution

ece5245

spastorino force-pushed the refactor-remap-lifetimes branch from b8ddba2 to ece5245 Compare August 4, 2022 15:40

Improve record_def_id_remap docs

bf1c7da

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 4, 2022

This comment has been minimized.

Sign in to view

spastorino added 2 commits August 4, 2022 15:13

Improve opt_local_def_id docs

065e497

Fix typo

4170d73

spastorino force-pushed the refactor-remap-lifetimes branch from fb7e44d to 4170d73 Compare August 4, 2022 18:14

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2022

bors merged commit cdfd675 into rust-lang:master Aug 5, 2022

rustbot added this to the 1.64.0 milestone Aug 5, 2022

bors mentioned this pull request Aug 5, 2022

hir::Generics: convert has_where_clause_predicates field into method #100113

Closed

aliemjay mentioned this pull request Aug 6, 2022

ICE: Region parameter out of range when substituting in region 'b (index=4) #100187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split create_def and lowering of lifetimes for opaque types and bare async fns #99867

Split create_def and lowering of lifetimes for opaque types and bare async fns #99867

spastorino commented Jul 28, 2022 •

edited

oli-obk commented Jul 29, 2022

rust-timer commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 29, 2022

cjgillot commented Jul 31, 2022

spastorino commented Aug 1, 2022

spastorino commented Aug 2, 2022

This comment has been minimized.

nikomatsakis Aug 2, 2022

nikomatsakis left a comment

nikomatsakis commented Aug 4, 2022

bors commented Aug 4, 2022

This comment has been minimized.

spastorino commented Aug 4, 2022

bors commented Aug 4, 2022

bors commented Aug 5, 2022

bors commented Aug 5, 2022

rust-timer commented Aug 5, 2022

Split create_def and lowering of lifetimes for opaque types and bare async fns #99867

Split create_def and lowering of lifetimes for opaque types and bare async fns #99867

Conversation

spastorino commented Jul 28, 2022 • edited

oli-obk commented Jul 29, 2022

rust-timer commented Jul 29, 2022

bors commented Jul 29, 2022

bors commented Jul 29, 2022

rust-timer commented Jul 29, 2022

rust-timer commented Jul 29, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

cjgillot commented Jul 31, 2022

spastorino commented Aug 1, 2022

spastorino commented Aug 2, 2022

This comment has been minimized.

nikomatsakis Aug 2, 2022

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 4, 2022

bors commented Aug 4, 2022

This comment has been minimized.

spastorino commented Aug 4, 2022

bors commented Aug 4, 2022

bors commented Aug 5, 2022

bors commented Aug 5, 2022

rust-timer commented Aug 5, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

spastorino commented Jul 28, 2022 •

edited