Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully serialize AdtDef #91924

Merged
merged 3 commits into from
Dec 20, 2021
Merged

Fully serialize AdtDef #91924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5f9054
Fully serialize AdtDef
Aaron1011 Dec 14, 2021
70fba90
Cache adt_def query result on disk
Aaron1011 Dec 15, 2021
00ce6dc
Only cache local adt_def results on disk
Aaron1011 Dec 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/arena.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ macro_rules! arena_types {
[] layout: rustc_target::abi::Layout,
[] fn_abi: rustc_target::abi::call::FnAbi<'tcx, rustc_middle::ty::Ty<'tcx>>,
// AdtDef are interned and compared by address
[] adt_def: rustc_middle::ty::AdtDef,
[decode] adt_def: rustc_middle::ty::AdtDef,
[] steal_thir: rustc_data_structures::steal::Steal<rustc_middle::thir::Thir<'tcx>>,
[] steal_mir: rustc_data_structures::steal::Steal<rustc_middle::mir::Body<'tcx>>,
[decode] mir: rustc_middle::mir::Body<'tcx>,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ rustc_queries! {
}
query adt_def(key: DefId) -> &'tcx ty::AdtDef {
desc { |tcx| "computing ADT definition for `{}`", tcx.def_path_str(key) }
cache_on_disk_if { key.is_local() }
separate_provide_extern
}
query adt_destructor(key: DefId) -> Option<ty::Destructor> {
Expand Down
23 changes: 10 additions & 13 deletions compiler/rustc_middle/src/ty/adt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_index::vec::{Idx, IndexVec};
use rustc_query_system::ich::StableHashingContext;
use rustc_serialize::{self, Encodable, Encoder};
use rustc_session::DataTypeKind;
use rustc_span::symbol::sym;
use rustc_target::abi::VariantIdx;
Expand All @@ -20,7 +19,7 @@ use std::cell::RefCell;
use std::cmp::Ordering;
use std::hash::{Hash, Hasher};
use std::ops::Range;
use std::{ptr, str};
use std::str;

use super::{
Destructor, FieldDef, GenericPredicates, ReprOptions, Ty, TyCtxt, VariantDef, VariantDiscr,
Expand All @@ -30,7 +29,7 @@ use super::{
pub struct AdtSizedConstraint<'tcx>(pub &'tcx [Ty<'tcx>]);

bitflags! {
#[derive(HashStable)]
#[derive(HashStable, TyEncodable, TyDecodable)]
pub struct AdtFlags: u32 {
const NO_ADT_FLAGS = 0;
/// Indicates whether the ADT is an enum.
Expand Down Expand Up @@ -88,6 +87,7 @@ bitflags! {
///
/// where `x` here represents the `DefId` of `S.x`. Then, the `DefId`
/// can be used with [`TyCtxt::type_of()`] to get the type of the field.
#[derive(TyEncodable, TyDecodable)]
pub struct AdtDef {
/// The `DefId` of the struct, enum or union item.
pub did: DefId,
Expand All @@ -113,26 +113,23 @@ impl Ord for AdtDef {
}
}

/// There should be only one AdtDef for each `did`, therefore
/// it is fine to implement `PartialEq` only based on `did`.
impl PartialEq for AdtDef {
// `AdtDef`s are always interned, and this is part of `TyS` equality.
#[inline]
fn eq(&self, other: &Self) -> bool {
ptr::eq(self, other)
self.did == other.did
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we keep the pointer equality here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using pointer equality breaks compilation of stage1 libc: I get errors like:

error[E0308]: mismatched types
   --> /home/aaron/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/compiler_builtins-0.1.65/src/int/specialized_div_rem/mod.rs:125:12
    |
125 |     if let Some(quo) = duo.checked_div(div) {
    |            ^^^^^^^^^   -------------------- this expression has type `Option<u64>`
    |            |
    |            expected enum `Option`, found a different enum `Option`
    |
    = note: expected enum `Option<u64>`
               found enum `Option<_>`

I believe we need a 'normal' PartialEq impl in order for interning to work properly, and we need interning because current code is relying on pointers to an AdtDef being unique (since they all currently get allocated through a query).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need a 'normal' PartialEq impl in order for interning to work properly

if you have the time, could you try a perf run which only removes the Hash and Eq impl and does not touch any queries? Would be great to test that before merging this PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #92040 for a perf run with just Hash and PartialEq changed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the perf results, it seems that the performance impact is acceptable.

}
}

impl Eq for AdtDef {}

/// There should be only one AdtDef for each `did`, therefore
/// it is fine to implement `Hash` only based on `did`.
impl Hash for AdtDef {
#[inline]
fn hash<H: Hasher>(&self, s: &mut H) {
(self as *const AdtDef).hash(s)
}
}

impl<S: Encoder> Encodable<S> for AdtDef {
fn encode(&self, s: &mut S) -> Result<(), S::Error> {
self.did.encode(s)
self.did.hash(s)
}
}

Expand Down Expand Up @@ -161,7 +158,7 @@ impl<'a> HashStable<StableHashingContext<'a>> for AdtDef {
}
}

#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, TyEncodable, TyDecodable)]
pub enum AdtKind {
Struct,
Union,
Expand Down
14 changes: 4 additions & 10 deletions compiler/rustc_middle/src/ty/codec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use crate::thir;
use crate::ty::subst::SubstsRef;
use crate::ty::{self, List, Ty, TyCtxt};
use rustc_data_structures::fx::FxHashMap;
use rustc_hir::def_id::DefId;
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
use rustc_span::Span;
use std::hash::Hash;
Expand Down Expand Up @@ -156,7 +155,8 @@ encodable_via_deref! {
&'tcx mir::Body<'tcx>,
&'tcx mir::UnsafetyCheckResult,
&'tcx mir::BorrowCheckResult<'tcx>,
&'tcx mir::coverage::CodeRegion
&'tcx mir::coverage::CodeRegion,
&'tcx ty::AdtDef
}

pub trait TyDecoder<'tcx>: Decoder {
Expand Down Expand Up @@ -308,13 +308,6 @@ macro_rules! impl_decodable_via_ref {
}
}

impl<'tcx, D: TyDecoder<'tcx>> RefDecodable<'tcx, D> for ty::AdtDef {
fn decode(decoder: &mut D) -> Result<&'tcx Self, D::Error> {
let def_id = <DefId as Decodable<D>>::decode(decoder)?;
Ok(decoder.tcx().adt_def(def_id))
}
}

impl<'tcx, D: TyDecoder<'tcx>> RefDecodable<'tcx, D> for ty::List<Ty<'tcx>> {
fn decode(decoder: &mut D) -> Result<&'tcx Self, D::Error> {
let len = decoder.read_usize()?;
Expand Down Expand Up @@ -399,7 +392,8 @@ impl_decodable_via_ref! {
&'tcx mir::UnsafetyCheckResult,
&'tcx mir::BorrowCheckResult<'tcx>,
&'tcx mir::coverage::CodeRegion,
&'tcx ty::List<ty::BoundVariableKind>
&'tcx ty::List<ty::BoundVariableKind>,
&'tcx ty::AdtDef
}

#[macro_export]
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ pub struct CtxtInterners<'tcx> {
const_allocation: InternedSet<'tcx, Allocation>,
bound_variable_kinds: InternedSet<'tcx, List<ty::BoundVariableKind>>,
layout: InternedSet<'tcx, Layout>,
adt_def: InternedSet<'tcx, AdtDef>,
}

impl<'tcx> CtxtInterners<'tcx> {
Expand All @@ -132,6 +133,7 @@ impl<'tcx> CtxtInterners<'tcx> {
const_allocation: Default::default(),
bound_variable_kinds: Default::default(),
layout: Default::default(),
adt_def: Default::default(),
}
}

Expand Down Expand Up @@ -1078,7 +1080,7 @@ impl<'tcx> TyCtxt<'tcx> {
variants: IndexVec<VariantIdx, ty::VariantDef>,
repr: ReprOptions,
) -> &'tcx ty::AdtDef {
self.arena.alloc(ty::AdtDef::new(self, did, kind, variants, repr))
self.intern_adt_def(ty::AdtDef::new(self, did, kind, variants, repr))
}

/// Allocates a read-only byte or string literal for `mir::interpret`.
Expand Down Expand Up @@ -2057,6 +2059,7 @@ direct_interners! {
const_: mk_const(Const<'tcx>),
const_allocation: intern_const_alloc(Allocation),
layout: intern_layout(Layout),
adt_def: intern_adt_def(AdtDef),
}

macro_rules! slice_interners {
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1472,7 +1472,7 @@ pub struct Destructor {
}

bitflags! {
#[derive(HashStable)]
#[derive(HashStable, TyEncodable, TyDecodable)]
pub struct VariantFlags: u32 {
const NO_VARIANT_FLAGS = 0;
/// Indicates whether the field list of this variant is `#[non_exhaustive]`.
Expand All @@ -1484,7 +1484,7 @@ bitflags! {
}

/// Definition of a variant -- a struct's fields or an enum variant.
#[derive(Debug, HashStable)]
#[derive(Debug, HashStable, TyEncodable, TyDecodable)]
pub struct VariantDef {
/// `DefId` that identifies the variant itself.
/// If this variant belongs to a struct or union, then this is a copy of its `DefId`.
Expand Down Expand Up @@ -1586,7 +1586,7 @@ pub enum VariantDiscr {
Relative(u32),
}

#[derive(Debug, HashStable)]
#[derive(Debug, HashStable, TyEncodable, TyDecodable)]
pub struct FieldDef {
pub did: DefId,
#[stable_hasher(project(name))]
Expand Down