Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

eddyb · 2020-07-14T19:28:23Z

Based on #74314, which removes the main use of synthetic type parameters in closure/generator ty::Generics

This PR keeps the "synthetics" (closure kind/signature/upvar types, or similar information for generators) only in the Ty (Closure/Generator) substitutions (used during inference and for type layout), but not in Instances which are used to refer to the MIR body of the closure during codegen (and the MIR body doesn't refer to the "synthetics" at all).

I originally started work on this to change the codegen behavior of closures with no type/const generics in scope (and have them not be deduplicated across crates), as per #74283 (comment) - but have since given up on that line of experimentation.

It might still improve some performance or help with @davidtwco polymorphization work, but it's not clear that it will.

r? @nikomatsakis

eddyb · 2020-07-14T19:28:38Z

@bors try @rust-timer queue

rust-timer · 2020-07-14T19:28:40Z

Awaiting bors try build completion

bors · 2020-07-14T19:28:50Z

⌛ Trying commit 6e4b8ef with merge fb73d5197a7ff8fff33c852694de43fdca79aa0a...

eddyb · 2020-07-14T19:31:49Z

src/librustc_typeck/collect.rs

@@ -224,7 +224,6 @@ impl Visitor<'tcx> for CollectItemTypesVisitor<'tcx> {
        if let hir::ExprKind::Closure(..) = expr.kind {
            let def_id = self.tcx.hir().local_def_id(expr.hir_id);
            self.tcx.ensure().generics_of(def_id);
-            self.tcx.ensure().type_of(def_id);


I removed this line to make 29 UI tests pass - their only failure mode was missing some of the errors.

I believe what's happening is that this would now trigger type-checking the body of the closure, before bodies are normally type-checked, and so it trips on an "abort if any errors" check.

Can we start removing those "checkpoints" that turn regular errors into fatal ones, given how on-demand some of the various checks are nowadays? cc @estebank

I think there are only a handful of FatalError.raise()s left, and the ones that do remain do so because they'd require a much larger refactor to get rid of them.

If they're because of having to return a value, I suspect using Result<T, ErrorReported> could work well.

(But I meant "abort if there were any errors" "checkpoints", which we can just remove)

bors · 2020-07-14T21:36:17Z

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

rust-timer · 2020-07-14T21:36:19Z

Queued fb73d5197a7ff8fff33c852694de43fdca79aa0a with parent c724b67, future comparison URL.

rust-timer · 2020-07-15T04:44:29Z

Finished benchmarking try commit (fb73d5197a7ff8fff33c852694de43fdca79aa0a): comparison url.

eddyb · 2020-07-15T08:10:10Z

Most of the wins are in some "patched incremental" benchmarks doing less codegen, I'm guessing something about a closure changes and it previously was treated as the identity of the closure instance changing, which was unnecessary and wasteful.

Do note that the numbers are all "patched incremental", with negligible effects on anything else. Maybe rustc-perf should emphasize that split more? Right now the use of only relative values can make it less clear what's going on.
cc @Mark-Simulacrum

eddyb · 2020-07-16T15:59:48Z

Just noticed #69749 adds "parent substs" to the Split{Closure,Generator}Generics, I should do that for this and #74314.

nikomatsakis · 2020-07-17T20:09:19Z

src/librustc_typeck/collect/type_of.rs

-                tcx.mk_closure(def_id, substs)
-            }
+        Node::Expr(&Expr { kind: ExprKind::Closure(..), .. }) => {
+            tcx.typeck_tables_of(def_id.expect_local()).node_type(hir_id)


This feels wrong to me. We basically undid a move much like this for generators, though partly it was to avoid those type error problems you were referring to with your other change.

But also it is just surprising to me to have type_of require us to type-check the closure. For every other def-id, getting type_of doesn't require a full type-check, but rather returns a "fully generic" type. i.e., I would expet the type of a closure or generator ought to be expressed as a purely generic type like Closure<P0...Pn>, much like we'd expect for a function, not one that embeds full types like Closure<..., (u32, u64)> or whatever. Do we have any other example that works like that?

Anyway, what is the exact motivation here? It's confusing to me :)

Ah, ok, I see, the point is that you want Instance to be able to substitute only the base_params and to still get a valid type... hmm. Interesting. I'm not sure what I think about it. I'm surprised this gives much of a win, can you explain why it does?

If we do land this, I feel like we need some kind of comment..somewhere...that explains this rather subtle point. I guess it would be best to document it on the ClosureSubsts and GeneratorSubsts structs.

Ah, ok, I see, the point is that you want Instance to be able to substitute only the base_params and to still get a valid type... hmm. Interesting. I'm not sure what I think about it.

Me neither, I think I'd prefer if we could implement e.g. fn_sig on closures instead, and use that from Instance.
I could look into that approach once #74314 lands.

I'm surprised this gives much of a win, can you explain why it does?

All the wins are incremental, so it probably only makes sense if a closure changes its capture set/types, although that would still imply codegen is needed?

Or maybe non-generic closures are involved, which would now result in an Instance with empty substs, which may be handled differently by CGUs.

LeSeulArtichaut · 2020-08-22T17:30:42Z

Ping from triage: @nikomatsakis I believe this PR needs your review?

Me neither, I think I'd prefer if we could implement e.g. fn_sig on closures instead, and use that from Instance.
I could look into that approach once #74314 lands.

@eddyb I'm seeing that #74314 has landed now, did you have time to look into this?

camelid · 2020-10-07T17:00:41Z

(This is now S-waiting-on-review per new triage guidance.)

EDIT: Oops, I'm new to triage.

nikomatsakis · 2020-10-15T18:12:34Z

I'm still vaguely against this PR, because it makes closures/generators "force" type check to complete just to do type_of, which feels surprising to me and non-analogous with structs. I'd like to know whether @eddyb has an alternative approach in mind -- I don't quite understand what they meant regarding fn_sig on closures.

nikomatsakis · 2020-11-19T14:40:20Z

I'm going to close this PR for inactivity. @eddyb if you want to discuss this approach (or the fn_sig changes you alluded to), ping me on Zulip and let's schedule a time.

eddyb added 2 commits July 14, 2020 09:46

rustc_typeck: construct {Closure,Generator}Substs more directly.

5533705

Don't keep {Closure,Generator}Substs synthetics in an Instance.

6e4b8ef

rust-highfive assigned nikomatsakis Jul 14, 2020

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

eddyb commented Jul 14, 2020

View reviewed changes

eddyb mentioned this pull request Jul 16, 2020

rustc_typeck: construct {Closure,Generator}Substs more directly. #74314

Merged

nikomatsakis reviewed Jul 17, 2020

View reviewed changes

Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2020

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2020

Dylan-DPC-zz 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 Sep 12, 2020

JohnCSimon removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2020

JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 3, 2020

nikomatsakis closed this Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

eddyb commented Jul 14, 2020

eddyb commented Jul 14, 2020

rust-timer commented Jul 14, 2020

bors commented Jul 14, 2020

eddyb Jul 14, 2020

estebank Jul 14, 2020

eddyb Jul 14, 2020 •

edited

Loading

bors commented Jul 14, 2020

rust-timer commented Jul 14, 2020

rust-timer commented Jul 15, 2020

eddyb commented Jul 15, 2020

eddyb commented Jul 16, 2020

nikomatsakis Jul 17, 2020

nikomatsakis Jul 17, 2020

nikomatsakis Jul 17, 2020

eddyb Jul 19, 2020

LeSeulArtichaut commented Aug 22, 2020

camelid commented Oct 7, 2020 •

edited

Loading

nikomatsakis commented Oct 15, 2020

nikomatsakis commented Nov 19, 2020

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

Don't keep {Closure,Generator}Substs synthetics in an Instance. #74341

Conversation

eddyb commented Jul 14, 2020

eddyb commented Jul 14, 2020

rust-timer commented Jul 14, 2020

bors commented Jul 14, 2020

eddyb Jul 14, 2020

Choose a reason for hiding this comment

estebank Jul 14, 2020

Choose a reason for hiding this comment

eddyb Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Jul 14, 2020

rust-timer commented Jul 14, 2020

rust-timer commented Jul 15, 2020

eddyb commented Jul 15, 2020

eddyb commented Jul 16, 2020

nikomatsakis Jul 17, 2020

Choose a reason for hiding this comment

nikomatsakis Jul 17, 2020

Choose a reason for hiding this comment

nikomatsakis Jul 17, 2020

Choose a reason for hiding this comment

eddyb Jul 19, 2020

Choose a reason for hiding this comment

LeSeulArtichaut commented Aug 22, 2020

camelid commented Oct 7, 2020 • edited Loading

nikomatsakis commented Oct 15, 2020

nikomatsakis commented Nov 19, 2020

eddyb Jul 14, 2020 •

edited

Loading

camelid commented Oct 7, 2020 •

edited

Loading