rustc: keep upvars tupled in {Closure,Generator}Substs. #69968

eddyb · 2020-03-13T02:24:33Z

Previously, each closure/generator capture's (aka "upvar") type was tracked as one "synthetic" type parameter in the closure/generator substs, and figuring out where the parent fn's generics end and the synthetics start involved slicing at tcx.generics_of(def_id).parent_count.

Needing to query generics_of limited @davidtwco (who wants to compute some TypeFlags differently for parent generics vs upvars, and TyCtxt is not available there), which is how I got started on this, but it's also possible that the generics_of queries are slowing down {Closure,Generator}Substs methods.

To give an example, for a foo::<T, U>::{closure#0} with captures x: X and y: Y, substs are:

before this PR: [T, U, /*kind*/, /*signature*/, X, Y]
after this PR: [T, U, /*kind*/, /*signature*/, (X, Y)]

You can see that, with this PR, no matter how many captures, the last 3 entries in the substs (or 5 for a generator) are always the "synthetic" ones, with the last one being the tuple of capture types.

r? @nikomatsakis cc @Zoxc

eddyb · 2020-03-13T02:52:20Z

@bors try @rust-timer queue

rust-timer · 2020-03-13T02:52:21Z

Awaiting bors try build completion

bors · 2020-03-13T02:52:36Z

⌛ Trying commit da30907 with merge 6429062...

@davidtwco

rustc: keep upvars tupled in {Closure,Generator}Substs. Previously, each closure/generator capture's (aka "upvar") type was tracked as one "synthetic" type parameter in the closure/generator substs, and figuring out where the parent `fn`'s generics end and the synthetics start involved slicing at `tcx.generics_of(def_id).parent_count`. Needing to query `generics_of` limited @davidtwco (who wants to compute some `TypeFlags` differently for parent generics vs upvars, and `TyCtxt` is not available there), which is how I got started on this, but it's also possible that the `generics_of` queries are slowing down `{Closure,Generator}Substs` methods. To give an example, for a `foo::<T, U>::{closure#0}` with captures `x: X` and `y: Y`, substs are: * before this PR: `[T, U, /*kind*/, /*signature*/, X, Y]` * after this PR: `[T, U, /*kind*/, /*signature*/, (X, Y)]` You can see that, with this PR, no matter how many captures, the last 3 entries in the substs (or 5 for a generator) are always the "synthetic" ones, with the last one being the tuple of capture types. r? @nikomatsakis cc @Zoxc

eddyb · 2020-03-13T02:54:37Z

src/test/ui/issues/issue-22638.stderr

-   = note: consider adding a `#![type_length_limit="26214380"]` attribute to your crate
+   = note: consider adding a `#![type_length_limit="30408681"]` attribute to your crate


@rust-lang/compiler This is a bit silly, can we remove type_length_limit?
Does it do anything for us nowadays? Other than slow down compilation.

I assume by "remove" you mean "make it do nothing"?

Oh, right, it's stable. The other thing we can do is count only unique types, which should make cases like these far less drastic presumably.

I'm confused -- why would it not do something for us? There are various cases where the size of types and amount of computational work can balloon without generating large stacks (also true: in the SLG solver Chalk at least, we don't even have a concept of overflow apart from type-size limits, though depending on what precise solver design we wind up with, we could keep something like overflow, but it's not a great concept because it makes results for a given query dependent on context instead of independent).

That said, this is an orthogonal place where we could apply type-limits, instead of applying them at type creation time or whatever.

I think it was added originally to not generate LLVM type names that were too long or something, which is a silly problem IMO.

At the very least we should switch to a DAG approach (i.e. don't count duplicates), I assume Chalk at least is similarly proportional in unique types, not total occurrences.

I'm not sure what duplicates exactly means? Oh, I guess like Foo<(A, A)> only counts A once? Chalk is currently just counting the total number of types I think. I'd have to think if the DAG version "suffices". I'm not sure, why is that much better? I'm just a bit confused about why you care about this I guess. Is it something that you've seen arise frequently?

Is it something that you've seen arise frequently?

It's almost guaranteed to arise with closures in generic functions, the generic shows up once in the parent Substs and then in one or more capture types.

That multiplies during monomorphization, and can blow out of proportions, despite nothing actually operating proportionally to the multiple occurrences.

Remember #54540? 🤣

src/librustc_typeck/check/closure.rs

bors · 2020-03-13T07:07:36Z

💥 Test timed out

eddyb · 2020-03-13T11:21:09Z

@bors try

bors · 2020-03-13T11:21:23Z

⌛ Trying commit da30907 with merge 26cab41b863fc625c0d81f771e063c685680f335...

src/librustc/ty/sty.rs

davidtwco · 2020-03-13T12:22:07Z

src/librustc/ty/sty.rs

-    fn split(self, def_id: DefId, tcx: TyCtxt<'_>) -> SplitClosureSubsts<'tcx> {
-        let generics = tcx.generics_of(def_id);
-        let parent_len = generics.parent_count;
+    fn split(self) -> SplitClosureSubsts<'tcx> {


I've already mentioned this to @eddyb via PM, but it could be useful for SplitClosureSubsts to contain a parent_substs: &'tcx [GenericArg<'tcx>] field - that way any callers that need to access the specific components of the closure substs (and use SplitClosureSubsts to do so) can also access the parent substs without having to reproduce some of this logic.

You can add this in your PR, FWIW, but if @nikomatsakis doesn't mind I can do it in this one too.

bors · 2020-03-13T15:36:23Z

💥 Test timed out

eddyb · 2020-03-13T16:07:17Z

@bors try (CI outage should be fixed now)

bors · 2020-03-13T16:07:27Z

⌛ Trying commit da30907 with merge f7b1e868f01ad49dcc05daad583793f5c413bc9a...

bors · 2020-03-13T18:39:59Z

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

rust-timer · 2020-03-13T18:40:00Z

Queued f7b1e868f01ad49dcc05daad583793f5c413bc9a with parent 54b7d21, future comparison URL.

@davidtwco

rustc: keep upvars tupled in {Closure,Generator}Substs. Previously, each closure/generator capture's (aka "upvar") type was tracked as one "synthetic" type parameter in the closure/generator substs, and figuring out where the parent `fn`'s generics end and the synthetics start involved slicing at `tcx.generics_of(def_id).parent_count`. Needing to query `generics_of` limited @davidtwco (who wants to compute some `TypeFlags` differently for parent generics vs upvars, and `TyCtxt` is not available there), which is how I got started on this, but it's also possible that the `generics_of` queries are slowing down `{Closure,Generator}Substs` methods. To give an example, for a `foo::<T, U>::{closure#0}` with captures `x: X` and `y: Y`, substs are: * before this PR: `[T, U, /*kind*/, /*signature*/, X, Y]` * after this PR: `[T, U, /*kind*/, /*signature*/, (X, Y)]` You can see that, with this PR, no matter how many captures, the last 3 entries in the substs (or 5 for a generator) are always the "synthetic" ones, with the last one being the tuple of capture types. r? @nikomatsakis cc @Zoxc

…s sense.

eddyb · 2020-03-21T12:51:06Z

@bors r=nikomatsakis

bors · 2020-03-21T12:51:07Z

📌 Commit d9a15cc has been approved by nikomatsakis

@ghost

Rollup of 8 pull requests Successful merges: - rust-lang#69080 (rustc_codegen_llvm: don't generate any type debuginfo for -Cdebuginfo=1.) - rust-lang#69940 (librustc_codegen_llvm: Replace deprecated API usage) - rust-lang#69942 (Increase verbosity when suggesting subtle code changes) - rust-lang#69968 (rustc: keep upvars tupled in {Closure,Generator}Substs.) - rust-lang#70123 (Ensure LLVM is in the link path for rustc tools) - rust-lang#70159 (Update the bundled wasi-libc with libstd) - rust-lang#70233 (resolve: Do not resolve visibilities on proc macro definitions twice) - rust-lang#70286 (Miri error type: remove UbExperimental variant) Failed merges: r? @ghost

rustup rust-lang/rust#69968 changelog: none

Changes: ```` rustup rust-lang#69968 Fix documentation generation for configurable lints Fix single binding in closure Improvement: Don't show function body in needless_lifetimes ````

@Dylan-DPC

submodules: update clippy from d8e6e4c to 1ff81c1 Changes: ```` rustup rust-lang#69968 Fix documentation generation for configurable lints Fix single binding in closure Improvement: Don't show function body in needless_lifetimes ```` Fixes rust-lang#70310 r? @Dylan-DPC

Changes: ```` rustup rust-lang/rust#69968 Fix documentation generation for configurable lints Fix single binding in closure Improvement: Don't show function body in needless_lifetimes ````

rust-highfive assigned nikomatsakis Mar 13, 2020

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

eddyb commented Mar 13, 2020

View reviewed changes

Centril reviewed Mar 13, 2020

View reviewed changes

src/librustc_typeck/check/closure.rs Outdated Show resolved Hide resolved

oli-obk reviewed Mar 13, 2020

View reviewed changes

davidtwco reviewed Mar 13, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

davidtwco mentioned this pull request Mar 14, 2020

Polymorphization #69749

Merged

eddyb force-pushed the tupled-closure-captures branch 2 times, most recently from ca8521b to cb6cdc0 Compare March 15, 2020 15:44

This comment has been minimized.

Sign in to view

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 Mar 21, 2020

eddyb added 3 commits March 21, 2020 14:23

rustc: keep upvars tupled in {Closure,Generator}Substs.

10f08ab

rustc: make {Closure,Generator}Substs::split as cheap as possible.

3fd397b

rustc_typeck: move comment about inference variables to where it make…

d9a15cc

…s sense.

eddyb force-pushed the tupled-closure-captures branch from 7c39021 to d9a15cc Compare March 21, 2020 12:50

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 Mar 21, 2020

Centril mentioned this pull request Mar 23, 2020

Rollup of 8 pull requests #70305

Merged

bors merged commit bee074f into rust-lang:master Mar 23, 2020

matthiaskrgr added a commit to matthiaskrgr/rust-clippy that referenced this pull request Mar 23, 2020

rustup rust-lang/rust#69968

0f31951

bors added a commit to rust-lang/rust-clippy that referenced this pull request Mar 23, 2020

Auto merge of #5361 - matthiaskrgr:rustup_37, r=flip1995

43bdee0

rustup rust-lang/rust#69968 changelog: none

eddyb deleted the tupled-closure-captures branch March 23, 2020 14:48

eddyb mentioned this pull request Mar 24, 2020

Make the type_of return a generic type for generators #68884

Merged

blitzerr mentioned this pull request Jun 12, 2020

Always make the "upvar types" a tuple of the actual upvar types (Parent issue#53488) rust-lang/project-rfc-2229#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: keep upvars tupled in {Closure,Generator}Substs. #69968

rustc: keep upvars tupled in {Closure,Generator}Substs. #69968

eddyb commented Mar 13, 2020

eddyb commented Mar 13, 2020

rust-timer commented Mar 13, 2020

bors commented Mar 13, 2020

eddyb Mar 13, 2020

Centril Mar 13, 2020

eddyb Mar 13, 2020

nikomatsakis Mar 19, 2020

nikomatsakis Mar 19, 2020

eddyb Mar 19, 2020

nikomatsakis Mar 19, 2020

eddyb Mar 19, 2020

bors commented Mar 13, 2020

eddyb commented Mar 13, 2020

bors commented Mar 13, 2020

davidtwco Mar 13, 2020

eddyb Mar 13, 2020

bors commented Mar 13, 2020

eddyb commented Mar 13, 2020

bors commented Mar 13, 2020

bors commented Mar 13, 2020

rust-timer commented Mar 13, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Mar 21, 2020

bors commented Mar 21, 2020

		= note: consider adding a `#![type_length_limit="26214380"]` attribute to your crate
		= note: consider adding a `#![type_length_limit="30408681"]` attribute to your crate

rustc: keep upvars tupled in {Closure,Generator}Substs. #69968

rustc: keep upvars tupled in {Closure,Generator}Substs. #69968

Conversation

eddyb commented Mar 13, 2020

eddyb commented Mar 13, 2020

rust-timer commented Mar 13, 2020

bors commented Mar 13, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 13, 2020

eddyb commented Mar 13, 2020

bors commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 13, 2020

eddyb commented Mar 13, 2020

bors commented Mar 13, 2020

bors commented Mar 13, 2020

rust-timer commented Mar 13, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Mar 21, 2020

bors commented Mar 21, 2020