store ADT information in an AdtDef structure in a unified way

[arielb1]

This ended up being a bigger refactoring than I thought, as I also cleaned a few ugly points in rustc. There are still a few areas that need improvements.

Performance numbers:

Before:
572.70user 5.52system 7:33.21elapsed 127%CPU (0avgtext+0avgdata 1173368maxresident)k
llvm-time: 385.858

After:
545.27user 5.49system 7:10.22elapsed 128%CPU (0avgtext+0avgdata 1145348maxresident)k
llvm-time: 387.119

A good 5% perf improvement. Note that after this patch >70% of the time is spent in LLVM - Amdahl's law is in full effect.

Passes make check locally.

r? @nikomatsakis

[eefriedman]

I assume this slipped in by accident?

[arielb1]

yes

[arielb1]

passes check locally

[nikomatsakis]

Why require that T: Copy? I guess because in a concurrency-oriented future, that would be handy?

[nikomatsakis]

nit: move def.is_fundamental to the next line

[nikomatsakis]

In general, these flags could use comments, but definitely is_dtorck and is_dtorck_valid

[nikomatsakis]

Nit: I think Rust's precedent is to treat acronyms like other words. I'm sure we're inconsistent here and there, but Arc etc suggest so. Hence this should be AdtDef.

Less nit: Writing &'tcx AdtDef<'tcx> is tiresome and error-prone. We should at minimum follow the Ty precedent and make type AdtDef<'tcx> = &'tcx AdfDefData<'tcx>. I'd also personally be fine with a newtype (for both this and Ty).

[nikomatsakis]

(a new type feels right to me because this constraint is more naturally enforced as part of the newtype, which signals "an interned AdtDefData")

[arielb1]

When reverse-variance goes away this should be split into an lookup_adt_def_mut (for collect, which needs this kind of access) and lookup_adt_def (for everyone else), hand-variancing.

[nikomatsakis]

is this sentence incomplete? it'd be nice to have some documentation of where it is important to have variance here

[nikomatsakis]

If the major problem here is that we want Ty<'q> to be variant w/r/t 'q, perhaps it would be better to just have ty_struct continue to embed a DefId, rather than embedding a reference to the AdtDef?

OTOH, the trick you've worked out here is pretty clever. It does seem like it's nice to be able to have a "read-only" alias that is variant, and I guess that the lifetime games you are playing here make that possible.

/me ponders

[nikomatsakis]

I feel like we should find a more meaningful name than 'lt -- though it's tricky because it's sort of abstract. 'writer is sort of...ok, not a precise match with anything, but at least suggestive of the role of 'lt.

[nikomatsakis]

nit: can you replace skip_binder with a call to no_late_bound_regions and unwrap? I believe that is correctly because the tuple signature should have no LBR.

[nikomatsakis]

nice, I'm usually too lazy and just return a Vec :)

[nikomatsakis]

the commit message here suggests that changing how we issue this note causes a 2.5% perf improvement. How is that possible, given that this code only executes in error cases? What are you testing?

[nikomatsakis]

It'd be nice to move this code out of this file and (ideally) into dropck. Perhaps just have the code that uses the "is_dtorck" flag stuff elsewhere. Or maintain the flag a bit differently. Or use some external sets. I don't know. Not a huge thing, but I do feel like this kind of random code is best kept with the code that is consuming it (in general ty.rs has more stuff in it that I'd like).

[nikomatsakis]

eventually, this "destructor_for_type" thing should go into AdtDef, right?

[arielb1]

NOTE: this is commented to prevent duplicate error messages, and is uncommented in a later commit.

[Aatch]

Awesome, I'm glad somebody did this. The performance increase is consistent with what I saw with my version of this change. Hopefully this should also make working with ADTs easier going forward, at the very least it's much more obvious.

[nikomatsakis]

@arielb1

I copied somewhere that did .0 (you can't believe how many places do this). I match on the binder in several places - maybe I'll add a tuple_ctor_to_type function.

Oh, I can believe it, since I wrote them, but I've been migrating code to the following conventions:

1. Either use a proper function (like no_late_bound_regions) or,
2. If you call skip_binder, add a comment explaining why it makes sense in this particular case.

[nikomatsakis]

@arielb1

Having AdtDef<'tcx> be &'tcx AdtDef<'tcx,'tcx> is certainly nice. AdtDef_ is used basically nowhere (once in the tcx, once in decoder, once in collect), I'm not sure it should be behind a type alias. Also, this isn't really Mut - you can still fulfill an AdtDef<'tcx> with a Ty<'static>.

While technically true, that's not particularly interesting, is it? The intention of these types is as I wrote them, no? I'm fine if you want to note that caveat in a comment, but I still favor names that lay clear the intent of "read-only" vs "mutable", as this is a subtle trick.

[nikomatsakis]

@arielb1

dropck was somewhat an hotspot on my profile.

How is this relevant? I'm talking about what module the code is in, not what it does.

[nikomatsakis]

@Aatch

that's for the entire patchset up to this point (I wanted to separate it from Ty::is_simd).

ah, ok, sounds good.

[nikomatsakis]

cc @rust-lang/compiler <-- this PR is worth having more eyes on, incidentally

[nikomatsakis]

OK, I've finished reading through the PR. I am happy with it, this is nice work.

r+ modulo:

• rename AdtDef_ to AdtDefData or AdtDefStruct or something similar with a word, not an underscore
• readable type aliases for the (mostly) "read-only" &'tcx AdtDefData<'tcx,'static> and the mutable &'tcx AdtDefData<'tcx,'tcx>. Personally, I think it's worth giving both of them an alias because it makes the pattern clearer, even if the latter gets little use.
• remove or at minimum document calls to skip_binder
• explain where the T: Copy bound comes from (lets us use Cell; since we store &T it's fine; could be lifted in the future but would require more unsafe code). I was thinking that it mattered for concurrency but of course I was being silly: the whole POINT of an ivar is that it's write-once, and hence you can clone the contents without fear of them being overwritten, after all.

[nikomatsakis]

ps, if you prefer, perhaps the name AdtDefTcx or something would be a compromise vs AdtDefMut ("fulfillable with a Ty<'tcx>")

[arielb1]

addressed

[nikomatsakis]

Note: this still skips the binder :) it just does so in the implicit way I would eventually like to remove altogether (by making the 0 field private). But there is a comment and assert so...good enough.

[nikomatsakis]

@bors r+

[bors]

📌 Commit eedb1cc has been approved by nikomatsakis

[bors]

⌛️ Testing commit eedb1cc with merge ab77c1d...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt