From f09d474836d14811af1f9ebead4c1649c54e4e4c Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Tue, 3 Nov 2020 22:23:08 -0800 Subject: [PATCH 1/3] Use PackedFingerprint in DepNode to reduce memory consumption --- .../rustc_data_structures/src/fingerprint.rs | 42 +++++++++++++++++++ .../rustc_middle/src/dep_graph/dep_node.rs | 15 +++++-- .../src/dep_graph/dep_node.rs | 8 ++-- .../rustc_query_system/src/dep_graph/graph.rs | 4 +- 4 files changed, 60 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_data_structures/src/fingerprint.rs b/compiler/rustc_data_structures/src/fingerprint.rs index ec2f9597b18..9927511e3c1 100644 --- a/compiler/rustc_data_structures/src/fingerprint.rs +++ b/compiler/rustc_data_structures/src/fingerprint.rs @@ -151,8 +151,50 @@ impl FingerprintDecoder for D { panic!("Cannot decode `Fingerprint` with `{}`", std::any::type_name::()); } } + impl FingerprintDecoder for opaque::Decoder<'_> { fn decode_fingerprint(&mut self) -> Result { Fingerprint::decode_opaque(self) } } + +// `PackedFingerprint` wraps a `Fingerprint`. Its purpose is to, on certain +// architectures, behave like a `Fingerprint` without alignment requirements. +// This behavior is only enabled on x86 and x86_64, where the impact of +// unaligned accesses is tolerable in small doses. +// +// This may be preferable to use in large collections of structs containing +// fingerprints, as it can reduce memory consumption by preventing the padding +// that the more strictly-aligned `Fingerprint` can introduce. An application of +// this is in the query dependency graph, which contains a large collection of +// `DepNode`s. As of this writing, the size of a `DepNode` decreases by ~30% +// (from 24 bytes to 17) by using the packed representation here, which +// noticeably decreases total memory usage when compiling large crates. +#[cfg_attr(any(target_arch = "x86", target_arch = "x86_64"), repr(packed))] +#[derive(Eq, PartialEq, Ord, PartialOrd, Debug, Clone, Copy, Hash)] +pub struct PackedFingerprint(pub Fingerprint); + +impl std::fmt::Display for PackedFingerprint { + #[inline] + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Copy to avoid taking reference to packed field. + let copy = self.0; + copy.fmt(formatter) + } +} + +impl Encodable for PackedFingerprint { + #[inline] + fn encode(&self, s: &mut E) -> Result<(), E::Error> { + // Copy to avoid taking reference to packed field. + let copy = self.0; + copy.encode(s) + } +} + +impl Decodable for PackedFingerprint { + #[inline] + fn decode(d: &mut D) -> Result { + Fingerprint::decode(d).map(|f| PackedFingerprint(f)) + } +} diff --git a/compiler/rustc_middle/src/dep_graph/dep_node.rs b/compiler/rustc_middle/src/dep_graph/dep_node.rs index a61b9af9bac..9cd3689e897 100644 --- a/compiler/rustc_middle/src/dep_graph/dep_node.rs +++ b/compiler/rustc_middle/src/dep_graph/dep_node.rs @@ -61,7 +61,7 @@ use crate::ty::subst::{GenericArg, SubstsRef}; use crate::ty::{self, ParamEnvAnd, Ty, TyCtxt}; -use rustc_data_structures::fingerprint::Fingerprint; +use rustc_data_structures::fingerprint::{Fingerprint, PackedFingerprint}; use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, CRATE_DEF_INDEX}; use rustc_hir::definitions::DefPathHash; use rustc_hir::HirId; @@ -193,6 +193,15 @@ pub fn $variant(_tcx: TyCtxt<'_>, $(arg: $tuple_arg_ty)*) -> DepNode { pub type DepNode = rustc_query_system::dep_graph::DepNode; + // We keep a lot of `DepNode`s in memory during compilation. It's not + // required that their size stay the same, but we don't want to change + // it inadvertently. This assert just ensures we're aware of any change. + #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] + static_assert_size!(DepNode, 17); + + #[cfg(not(any(target_arch = "x86", target_arch = "x86_64")))] + static_assert_size!(DepNode, 24); + pub trait DepNodeExt: Sized { /// Construct a DepNode from the given DepKind and DefPathHash. This /// method will assert that the given DepKind actually requires a @@ -227,7 +236,7 @@ fn from_def_path_hash(def_path_hash: DefPathHash, kind: DepKind) -> DepNode { debug_assert!(kind.can_reconstruct_query_key() && kind.has_params()); DepNode { kind, - hash: def_path_hash.0, + hash: PackedFingerprint(def_path_hash.0), } } @@ -243,7 +252,7 @@ fn from_def_path_hash(def_path_hash: DefPathHash, kind: DepKind) -> DepNode { /// has been removed. fn extract_def_id(&self, tcx: TyCtxt<'tcx>) -> Option { if self.kind.can_reconstruct_query_key() { - let def_path_hash = DefPathHash(self.hash); + let def_path_hash = DefPathHash(self.hash.0); tcx.def_path_hash_to_def_id.as_ref()?.get(&def_path_hash).cloned() } else { None diff --git a/compiler/rustc_query_system/src/dep_graph/dep_node.rs b/compiler/rustc_query_system/src/dep_graph/dep_node.rs index 7808a28dff0..2ab3834468c 100644 --- a/compiler/rustc_query_system/src/dep_graph/dep_node.rs +++ b/compiler/rustc_query_system/src/dep_graph/dep_node.rs @@ -44,7 +44,7 @@ use super::{DepContext, DepKind}; -use rustc_data_structures::fingerprint::Fingerprint; +use rustc_data_structures::fingerprint::{Fingerprint, PackedFingerprint}; use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use std::fmt; @@ -53,7 +53,7 @@ #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Encodable, Decodable)] pub struct DepNode { pub kind: K, - pub hash: Fingerprint, + pub hash: PackedFingerprint, } impl DepNode { @@ -62,7 +62,7 @@ impl DepNode { /// does not require any parameters. pub fn new_no_params(kind: K) -> DepNode { debug_assert!(!kind.has_params()); - DepNode { kind, hash: Fingerprint::ZERO } + DepNode { kind, hash: PackedFingerprint(Fingerprint::ZERO) } } pub fn construct(tcx: Ctxt, kind: K, arg: &Key) -> DepNode @@ -71,7 +71,7 @@ pub fn construct(tcx: Ctxt, kind: K, arg: &Key) -> DepNode Key: DepNodeParams, { let hash = arg.to_fingerprint(tcx); - let dep_node = DepNode { kind, hash }; + let dep_node = DepNode { kind, hash: PackedFingerprint(hash) }; #[cfg(debug_assertions)] { diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index d9b687c48af..893017ac3bc 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -1,4 +1,4 @@ -use rustc_data_structures::fingerprint::Fingerprint; +use rustc_data_structures::fingerprint::{Fingerprint, PackedFingerprint}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::profiling::QueryInvocationId; use rustc_data_structures::sharded::{self, Sharded}; @@ -976,7 +976,7 @@ fn complete_anon_task(&self, kind: K, task_deps: TaskDeps) -> DepNodeIndex { // Fingerprint::combine() is faster than sending Fingerprint // through the StableHasher (at least as long as StableHasher // is so slow). - hash: self.anon_id_seed.combine(hasher.finish()), + hash: PackedFingerprint(self.anon_id_seed.combine(hasher.finish())), }; self.intern_node(target_dep_node, task_deps.reads, Fingerprint::ZERO) From 05dde137cad043738e94a47f63256f1e66c83768 Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Wed, 18 Nov 2020 15:10:43 -0800 Subject: [PATCH 2/3] Make PackedFingerprint's Fingerprint private --- .../rustc_data_structures/src/fingerprint.rs | 19 ++++++++++++++++++- .../rustc_middle/src/dep_graph/dep_node.rs | 6 +++--- .../src/dep_graph/dep_node.rs | 4 ++-- .../rustc_query_system/src/dep_graph/graph.rs | 4 ++-- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_data_structures/src/fingerprint.rs b/compiler/rustc_data_structures/src/fingerprint.rs index 9927511e3c1..01efcaf6f44 100644 --- a/compiler/rustc_data_structures/src/fingerprint.rs +++ b/compiler/rustc_data_structures/src/fingerprint.rs @@ -170,9 +170,12 @@ fn decode_fingerprint(&mut self) -> Result { // `DepNode`s. As of this writing, the size of a `DepNode` decreases by ~30% // (from 24 bytes to 17) by using the packed representation here, which // noticeably decreases total memory usage when compiling large crates. +// +// The wrapped `Fingerprint` is private to reduce the chance of a client +// invoking undefined behavior by taking a reference to the packed field. #[cfg_attr(any(target_arch = "x86", target_arch = "x86_64"), repr(packed))] #[derive(Eq, PartialEq, Ord, PartialOrd, Debug, Clone, Copy, Hash)] -pub struct PackedFingerprint(pub Fingerprint); +pub struct PackedFingerprint(Fingerprint); impl std::fmt::Display for PackedFingerprint { #[inline] @@ -198,3 +201,17 @@ fn decode(d: &mut D) -> Result { Fingerprint::decode(d).map(|f| PackedFingerprint(f)) } } + +impl From for PackedFingerprint { + #[inline] + fn from(f: Fingerprint) -> PackedFingerprint { + PackedFingerprint(f) + } +} + +impl From for Fingerprint { + #[inline] + fn from(f: PackedFingerprint) -> Fingerprint { + f.0 + } +} diff --git a/compiler/rustc_middle/src/dep_graph/dep_node.rs b/compiler/rustc_middle/src/dep_graph/dep_node.rs index 9cd3689e897..38bc3b46b0f 100644 --- a/compiler/rustc_middle/src/dep_graph/dep_node.rs +++ b/compiler/rustc_middle/src/dep_graph/dep_node.rs @@ -61,7 +61,7 @@ use crate::ty::subst::{GenericArg, SubstsRef}; use crate::ty::{self, ParamEnvAnd, Ty, TyCtxt}; -use rustc_data_structures::fingerprint::{Fingerprint, PackedFingerprint}; +use rustc_data_structures::fingerprint::Fingerprint; use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, CRATE_DEF_INDEX}; use rustc_hir::definitions::DefPathHash; use rustc_hir::HirId; @@ -236,7 +236,7 @@ fn from_def_path_hash(def_path_hash: DefPathHash, kind: DepKind) -> DepNode { debug_assert!(kind.can_reconstruct_query_key() && kind.has_params()); DepNode { kind, - hash: PackedFingerprint(def_path_hash.0), + hash: def_path_hash.0.into(), } } @@ -252,7 +252,7 @@ fn from_def_path_hash(def_path_hash: DefPathHash, kind: DepKind) -> DepNode { /// has been removed. fn extract_def_id(&self, tcx: TyCtxt<'tcx>) -> Option { if self.kind.can_reconstruct_query_key() { - let def_path_hash = DefPathHash(self.hash.0); + let def_path_hash = DefPathHash(self.hash.into()); tcx.def_path_hash_to_def_id.as_ref()?.get(&def_path_hash).cloned() } else { None diff --git a/compiler/rustc_query_system/src/dep_graph/dep_node.rs b/compiler/rustc_query_system/src/dep_graph/dep_node.rs index 2ab3834468c..3d9e739cd28 100644 --- a/compiler/rustc_query_system/src/dep_graph/dep_node.rs +++ b/compiler/rustc_query_system/src/dep_graph/dep_node.rs @@ -62,7 +62,7 @@ impl DepNode { /// does not require any parameters. pub fn new_no_params(kind: K) -> DepNode { debug_assert!(!kind.has_params()); - DepNode { kind, hash: PackedFingerprint(Fingerprint::ZERO) } + DepNode { kind, hash: Fingerprint::ZERO.into() } } pub fn construct(tcx: Ctxt, kind: K, arg: &Key) -> DepNode @@ -71,7 +71,7 @@ pub fn construct(tcx: Ctxt, kind: K, arg: &Key) -> DepNode Key: DepNodeParams, { let hash = arg.to_fingerprint(tcx); - let dep_node = DepNode { kind, hash: PackedFingerprint(hash) }; + let dep_node = DepNode { kind, hash: hash.into() }; #[cfg(debug_assertions)] { diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index 893017ac3bc..617ec84ae71 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -1,4 +1,4 @@ -use rustc_data_structures::fingerprint::{Fingerprint, PackedFingerprint}; +use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_data_structures::profiling::QueryInvocationId; use rustc_data_structures::sharded::{self, Sharded}; @@ -976,7 +976,7 @@ fn complete_anon_task(&self, kind: K, task_deps: TaskDeps) -> DepNodeIndex { // Fingerprint::combine() is faster than sending Fingerprint // through the StableHasher (at least as long as StableHasher // is so slow). - hash: PackedFingerprint(self.anon_id_seed.combine(hasher.finish())), + hash: self.anon_id_seed.combine(hasher.finish()).into(), }; self.intern_node(target_dep_node, task_deps.reads, Fingerprint::ZERO) From 142932ab197bbb0e1f64fa22d5a0d1c3e0fec83a Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Fri, 20 Nov 2020 01:13:15 -0800 Subject: [PATCH 3/3] Set unaligned_references lint to deny in rustc_data_structures To detect misuse of private packed field in `PackedFingerprint`. --- compiler/rustc_data_structures/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/rustc_data_structures/src/lib.rs b/compiler/rustc_data_structures/src/lib.rs index 6b952f20dd1..01604477c3e 100644 --- a/compiler/rustc_data_structures/src/lib.rs +++ b/compiler/rustc_data_structures/src/lib.rs @@ -31,6 +31,7 @@ #![feature(once_cell)] #![feature(maybe_uninit_uninit_array)] #![allow(rustc::default_hash_types)] +#![deny(unaligned_references)] #[macro_use] extern crate tracing;