Auto merge of rust-lang#139228 - petrochenkov:ctxtdecod5, r=<try>

hygiene: Remove all caching in syntax context decoding

rust-lang#129827 unintentionally removed one caching layer in syntax context decoding (rust-lang#129827 (comment)), and it was a perf regression.
However, it didn't remove all the infrastructure and locks supporting that caching layer.

Let's actually try to double down on that change, remove everything and see what happens.
If it doesn't work out, we'll try just try to re-land rust-lang#129827 without the `remapped_ctxts` removal.

cc `@bvanjoi`

Author: bors

compiler/rustc_metadata/src/rmeta/decoder.rs

@@ -31,7 +31,6 @@ use rustc_serialize::{Decodable, Decoder};
 use rustc_session::Session;
 use rustc_session::config::TargetModifier;
 use rustc_session::cstore::{CrateSource, ExternCrate};
-use rustc_span::hygiene::HygieneDecodeContext;
 use rustc_span::{BytePos, DUMMY_SP, Pos, SpanData, SpanDecoder, SyntaxContext, kw};
 use tracing::debug;
 
@@ -127,13 +126,6 @@ pub(crate) struct CrateMetadata {
     /// The crate was used non-speculatively.
     used: bool,
 
-    /// Additional data used for decoding `HygieneData` (e.g. `SyntaxContext`
-    /// and `ExpnId`).
-    /// Note that we store a `HygieneDecodeContext` for each `CrateMetadata`. This is
-    /// because `SyntaxContext` ids are not globally unique, so we need
-    /// to track which ids we've decoded on a per-crate basis.
-    hygiene_context: HygieneDecodeContext,
-
     // --- Data used only for improving diagnostics ---
     /// Information about the `extern crate` item or path that caused this crate to be loaded.
     /// If this is `None`, then the crate was injected (e.g., by the allocator).
@@ -470,7 +462,7 @@ impl<'a, 'tcx> SpanDecoder for DecodeContext<'a, 'tcx> {
         };
 
         let cname = cdata.root.name();
-        rustc_span::hygiene::decode_syntax_context(self, &cdata.hygiene_context, |_, id| {
+        rustc_span::hygiene::decode_syntax_context(self, |_, id| {
             debug!("SpecializedDecoder<SyntaxContext>: decoding {}", id);
             cdata
                 .root
@@ -1864,7 +1856,6 @@ impl CrateMetadata {
             host_hash,
             used: false,
             extern_crate: None,
-            hygiene_context: Default::default(),
             def_key_cache: Default::default(),
         };
 

compiler/rustc_metadata/src/rmeta/mod.rs

@@ -36,7 +36,7 @@ use rustc_serialize::opaque::FileEncoder;
 use rustc_session::config::{SymbolManglingVersion, TargetModifier};
 use rustc_session::cstore::{CrateDepKind, ForeignModule, LinkagePreference, NativeLib};
 use rustc_span::edition::Edition;
-use rustc_span::hygiene::{ExpnIndex, MacroKind, SyntaxContextData};
+use rustc_span::hygiene::{ExpnIndex, MacroKind, SyntaxContextKey};
 use rustc_span::{self, ExpnData, ExpnHash, ExpnId, Ident, Span, Symbol};
 use rustc_target::spec::{PanicStrategy, TargetTuple};
 use table::TableBuilder;
@@ -193,7 +193,7 @@ enum LazyState {
     Previous(NonZero<usize>),
 }
 
-type SyntaxContextTable = LazyTable<u32, Option<LazyValue<SyntaxContextData>>>;
+type SyntaxContextTable = LazyTable<u32, Option<LazyValue<SyntaxContextKey>>>;
 type ExpnDataTable = LazyTable<ExpnIndex, Option<LazyValue<ExpnData>>>;
 type ExpnHashTable = LazyTable<ExpnIndex, Option<LazyValue<ExpnHash>>>;
 

compiler/rustc_middle/src/query/on_disk_cache.rs

@@ -15,9 +15,7 @@ use rustc_query_system::query::QuerySideEffect;
 use rustc_serialize::opaque::{FileEncodeResult, FileEncoder, IntEncodedWithFixedSize, MemDecoder};
 use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
 use rustc_session::Session;
-use rustc_span::hygiene::{
-    ExpnId, HygieneDecodeContext, HygieneEncodeContext, SyntaxContext, SyntaxContextData,
-};
+use rustc_span::hygiene::{ExpnId, HygieneEncodeContext, SyntaxContext, SyntaxContextKey};
 use rustc_span::source_map::Spanned;
 use rustc_span::{
     BytePos, CachingSourceMapView, ExpnData, ExpnHash, Pos, RelativeBytePos, SourceFile, Span,
@@ -75,9 +73,9 @@ pub struct OnDiskCache {
     alloc_decoding_state: AllocDecodingState,
 
     // A map from syntax context ids to the position of their associated
-    // `SyntaxContextData`. We use a `u32` instead of a `SyntaxContext`
+    // `SyntaxContextKey`. We use a `u32` instead of a `SyntaxContext`
     // to represent the fact that we are storing *encoded* ids. When we decode
-    // a `SyntaxContext`, a new id will be allocated from the global `HygieneData`,
+    // a `SyntaxContextKey`, a new id will be allocated from the global `HygieneData`,
     // which will almost certainly be different than the serialized id.
     syntax_contexts: FxHashMap<u32, AbsoluteBytePos>,
     // A map from the `DefPathHash` of an `ExpnId` to the position
@@ -90,8 +88,6 @@ pub struct OnDiskCache {
     // we could look up the `ExpnData` from the metadata of foreign crates,
     // but it seemed easier to have `OnDiskCache` be independent of the `CStore`.
     expn_data: UnhashMap<ExpnHash, AbsoluteBytePos>,
-    // Additional information used when decoding hygiene data.
-    hygiene_context: HygieneDecodeContext,
     // Maps `ExpnHash`es to their raw value from the *previous*
     // compilation session. This is used as an initial 'guess' when
     // we try to map an `ExpnHash` to its value in the current
@@ -183,7 +179,6 @@ impl OnDiskCache {
             syntax_contexts: footer.syntax_contexts,
             expn_data: footer.expn_data,
             foreign_expn_data: footer.foreign_expn_data,
-            hygiene_context: Default::default(),
         })
     }
 
@@ -199,7 +194,6 @@ impl OnDiskCache {
             syntax_contexts: FxHashMap::default(),
             expn_data: UnhashMap::default(),
             foreign_expn_data: UnhashMap::default(),
-            hygiene_context: Default::default(),
         }
     }
 
@@ -305,7 +299,7 @@ impl OnDiskCache {
             let mut expn_data = UnhashMap::default();
             let mut foreign_expn_data = UnhashMap::default();
 
-            // Encode all hygiene data (`SyntaxContextData` and `ExpnData`) from the current
+            // Encode all hygiene data (`SyntaxContextKey` and `ExpnData`) from the current
             // session.
 
             hygiene_encode_context.encode(
@@ -428,7 +422,6 @@ impl OnDiskCache {
             syntax_contexts: &self.syntax_contexts,
             expn_data: &self.expn_data,
             foreign_expn_data: &self.foreign_expn_data,
-            hygiene_context: &self.hygiene_context,
         };
         f(&mut decoder)
     }
@@ -448,7 +441,6 @@ pub struct CacheDecoder<'a, 'tcx> {
     syntax_contexts: &'a FxHashMap<u32, AbsoluteBytePos>,
     expn_data: &'a UnhashMap<ExpnHash, AbsoluteBytePos>,
     foreign_expn_data: &'a UnhashMap<ExpnHash, u32>,
-    hygiene_context: &'a HygieneDecodeContext,
 }
 
 impl<'a, 'tcx> CacheDecoder<'a, 'tcx> {
@@ -561,12 +553,12 @@ impl<'a, 'tcx> Decodable<CacheDecoder<'a, 'tcx>> for Vec<u8> {
 impl<'a, 'tcx> SpanDecoder for CacheDecoder<'a, 'tcx> {
     fn decode_syntax_context(&mut self) -> SyntaxContext {
         let syntax_contexts = self.syntax_contexts;
-        rustc_span::hygiene::decode_syntax_context(self, self.hygiene_context, |this, id| {
+        rustc_span::hygiene::decode_syntax_context(self, |this, id| {
             // This closure is invoked if we haven't already decoded the data for the `SyntaxContext` we are deserializing.
             // We look up the position of the associated `SyntaxData` and decode it.
             let pos = syntax_contexts.get(&id).unwrap();
             this.with_position(pos.to_usize(), |decoder| {
-                let data: SyntaxContextData = decode_tagged(decoder, TAG_SYNTAX_CONTEXT);
+                let data: SyntaxContextKey = decode_tagged(decoder, TAG_SYNTAX_CONTEXT);
                 data
             })
         })

compiler/rustc_middle/src/ty/parameterized.rs

@@ -111,7 +111,7 @@ trivially_parameterized_over_tcx! {
     rustc_span::Span,
     rustc_span::Symbol,
     rustc_span::def_id::DefPathHash,
-    rustc_span::hygiene::SyntaxContextData,
+    rustc_span::hygiene::SyntaxContextKey,
     rustc_span::Ident,
     rustc_type_ir::Variance,
     rustc_hir::Attribute,

compiler/rustc_span/src/hygiene.rs

@@ -24,17 +24,14 @@
 // because getting it wrong can lead to nested `HygieneData::with` calls that
 // trigger runtime aborts. (Fortunately these are obvious and easy to fix.)
 
-use std::cell::RefCell;
-use std::collections::hash_map::Entry;
-use std::collections::hash_set::Entry as SetEntry;
 use std::hash::Hash;
 use std::sync::Arc;
 use std::{fmt, iter, mem};
 
 use rustc_data_structures::fingerprint::Fingerprint;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_data_structures::stable_hasher::{HashStable, HashingControls, StableHasher};
-use rustc_data_structures::sync::{Lock, WorkerLocal};
+use rustc_data_structures::sync::Lock;
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_hashes::Hash64;
 use rustc_index::IndexVec;
@@ -59,10 +56,10 @@ impl !PartialOrd for SyntaxContext {}
 
 /// If this part of two syntax contexts is equal, then the whole syntax contexts should be equal.
 /// The other fields are only for caching.
-type SyntaxContextKey = (SyntaxContext, ExpnId, Transparency);
+pub type SyntaxContextKey = (SyntaxContext, ExpnId, Transparency);
 
 #[derive(Clone, Copy, PartialEq, Debug, Encodable, Decodable)]
-pub struct SyntaxContextData {
+struct SyntaxContextData {
     outer_expn: ExpnId,
     outer_transparency: Transparency,
     parent: SyntaxContext,
@@ -1266,7 +1263,7 @@ impl HygieneEncodeContext {
     pub fn encode<T>(
         &self,
         encoder: &mut T,
-        mut encode_ctxt: impl FnMut(&mut T, u32, &SyntaxContextData),
+        mut encode_ctxt: impl FnMut(&mut T, u32, &SyntaxContextKey),
         mut encode_expn: impl FnMut(&mut T, ExpnId, &ExpnData, ExpnHash),
     ) {
         // When we serialize a `SyntaxContextData`, we may end up serializing
@@ -1286,9 +1283,9 @@ impl HygieneEncodeContext {
             // of the table that we insert data into doesn't depend on insertion
             // order
             #[allow(rustc::potential_query_instability)]
-            for_all_ctxts_in(latest_ctxts.into_iter(), |index, ctxt, data| {
+            for_all_ctxts_in(latest_ctxts.into_iter(), |index, ctxt, ctxt_key| {
                 if self.serialized_ctxts.lock().insert(ctxt) {
-                    encode_ctxt(encoder, index, data);
+                    encode_ctxt(encoder, index, ctxt_key);
                 }
             });
 
@@ -1306,29 +1303,6 @@ impl HygieneEncodeContext {
     }
 }
 
-#[derive(Default)]
-/// Additional information used to assist in decoding hygiene data
-struct HygieneDecodeContextInner {
-    // Maps serialized `SyntaxContext` ids to a `SyntaxContext` in the current
-    // global `HygieneData`. When we deserialize a `SyntaxContext`, we need to create
-    // a new id in the global `HygieneData`. This map tracks the ID we end up picking,
-    // so that multiple occurrences of the same serialized id are decoded to the same
-    // `SyntaxContext`. This only stores `SyntaxContext`s which are completely decoded.
-    remapped_ctxts: Vec<Option<SyntaxContext>>,
-
-    /// Maps serialized `SyntaxContext` ids that are currently being decoded to a `SyntaxContext`.
-    decoding: FxHashMap<u32, SyntaxContext>,
-}
-
-#[derive(Default)]
-/// Additional information used to assist in decoding hygiene data
-pub struct HygieneDecodeContext {
-    inner: Lock<HygieneDecodeContextInner>,
-
-    /// A set of serialized `SyntaxContext` ids that are currently being decoded on each thread.
-    local_in_progress: WorkerLocal<RefCell<FxHashSet<u32>>>,
-}
-
 /// Register an expansion which has been decoded from the on-disk-cache for the local crate.
 pub fn register_local_expn_id(data: ExpnData, hash: ExpnHash) -> ExpnId {
     HygieneData::with(|hygiene_data| {
@@ -1393,14 +1367,11 @@ pub fn decode_expn_id(
     register_expn_id(krate, index, expn_data, hash)
 }
 
-// Decodes `SyntaxContext`, using the provided `HygieneDecodeContext`
-// to track which `SyntaxContext`s we have already decoded.
-// The provided closure will be invoked to deserialize a `SyntaxContextData`
-// if we haven't already seen the id of the `SyntaxContext` we are deserializing.
-pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContextData>(
+// Decodes `SyntaxContext`.
+// The provided closure will be invoked to deserialize a `SyntaxContextData`.
+pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContextKey>(
     d: &mut D,
-    context: &HygieneDecodeContext,
-    decode_data: F,
+    decode_ctxt_key: F,
 ) -> SyntaxContext {
     let raw_id: u32 = Decodable::decode(d);
     if raw_id == 0 {
@@ -1409,115 +1380,21 @@ pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContext
         return SyntaxContext::root();
     }
 
-    let pending_ctxt = {
-        let mut inner = context.inner.lock();
-
-        // Reminder: `HygieneDecodeContext` is per-crate, so there are no collisions between
-        // raw ids from different crate metadatas.
-        if let Some(ctxt) = inner.remapped_ctxts.get(raw_id as usize).copied().flatten() {
-            // This has already been decoded.
-            return ctxt;
-        }
-
-        match inner.decoding.entry(raw_id) {
-            Entry::Occupied(ctxt_entry) => {
-                let pending_ctxt = *ctxt_entry.get();
-                match context.local_in_progress.borrow_mut().entry(raw_id) {
-                    // We're decoding this already on the current thread. Return here and let the
-                    // function higher up the stack finish decoding to handle recursive cases.
-                    // Hopefully having a `SyntaxContext` that refers to an incorrect data is ok
-                    // during reminder of the decoding process, it's certainly not ok after the
-                    // top level decoding function returns.
-                    SetEntry::Occupied(..) => return pending_ctxt,
-                    // Some other thread is currently decoding this.
-                    // Race with it (alternatively we could wait here).
-                    // We cannot return this value, unlike in the recursive case above, because it
-                    // may expose a `SyntaxContext` pointing to incorrect data to arbitrary code.
-                    SetEntry::Vacant(entry) => {
-                        entry.insert();
-                        pending_ctxt
-                    }
-                }
-            }
-            Entry::Vacant(entry) => {
-                // We are the first thread to start decoding. Mark the current thread as being
-                // progress.
-                context.local_in_progress.borrow_mut().insert(raw_id);
-
-                // Allocate and store SyntaxContext id *before* calling the decoder function,
-                // as the SyntaxContextData may reference itself.
-                let new_ctxt = HygieneData::with(|hygiene_data| {
-                    // Push a dummy SyntaxContextData to ensure that nobody else can get the
-                    // same ID as us. This will be overwritten after call `decode_data`.
-                    hygiene_data.syntax_context_data.push(SyntaxContextData::decode_placeholder());
-                    SyntaxContext::from_usize(hygiene_data.syntax_context_data.len() - 1)
-                });
-                entry.insert(new_ctxt);
-                new_ctxt
-            }
-        }
-    };
-
     // Don't try to decode data while holding the lock, since we need to
     // be able to recursively decode a SyntaxContext
-    let ctxt_data = decode_data(d, raw_id);
-    let ctxt_key = ctxt_data.key();
-
-    let ctxt = HygieneData::with(|hygiene_data| {
-        match hygiene_data.syntax_context_map.get(&ctxt_key) {
-            // Ensure that syntax contexts are unique.
-            // If syntax contexts with the given key already exists, reuse it instead of
-            // using `pending_ctxt`.
-            // `pending_ctxt` will leave an unused hole in the vector of syntax contexts.
-            // Hopefully its value isn't stored anywhere during decoding and its dummy data
-            // is never accessed later. The `is_decode_placeholder` asserts on all
-            // accesses to syntax context data attempt to ensure it.
-            Some(&ctxt) => ctxt,
-            // This is a completely new context.
-            // Overwrite its placeholder data with our decoded data.
-            None => {
-                let ctxt_data_ref =
-                    &mut hygiene_data.syntax_context_data[pending_ctxt.as_u32() as usize];
-                let prev_ctxt_data = mem::replace(ctxt_data_ref, ctxt_data);
-                // Reset `dollar_crate_name` so that it will be updated by `update_dollar_crate_names`.
-                // We don't care what the encoding crate set this to - we want to resolve it
-                // from the perspective of the current compilation session.
-                ctxt_data_ref.dollar_crate_name = kw::DollarCrate;
-                // Make sure nothing weird happened while `decode_data` was running.
-                if !prev_ctxt_data.is_decode_placeholder() {
-                    // Another thread may have already inserted the decoded data,
-                    // but the decoded data should match.
-                    assert_eq!(prev_ctxt_data, *ctxt_data_ref);
-                }
-                hygiene_data.syntax_context_map.insert(ctxt_key, pending_ctxt);
-                pending_ctxt
-            }
-        }
-    });
-
-    // Mark the context as completed
-    context.local_in_progress.borrow_mut().remove(&raw_id);
-
-    let mut inner = context.inner.lock();
-    let new_len = raw_id as usize + 1;
-    if inner.remapped_ctxts.len() < new_len {
-        inner.remapped_ctxts.resize(new_len, None);
-    }
-    inner.remapped_ctxts[raw_id as usize] = Some(ctxt);
-    inner.decoding.remove(&raw_id);
-
-    ctxt
+    let (parent, expn_id, transparency) = decode_ctxt_key(d, raw_id);
+    HygieneData::with(|hygiene_data| hygiene_data.alloc_ctxt(parent, expn_id, transparency))
 }
 
-fn for_all_ctxts_in<F: FnMut(u32, SyntaxContext, &SyntaxContextData)>(
+fn for_all_ctxts_in<F: FnMut(u32, SyntaxContext, &SyntaxContextKey)>(
     ctxts: impl Iterator<Item = SyntaxContext>,
     mut f: F,
 ) {
     let all_data: Vec<_> = HygieneData::with(|data| {
         ctxts.map(|ctxt| (ctxt, data.syntax_context_data[ctxt.0 as usize].clone())).collect()
     });
     for (ctxt, data) in all_data.into_iter() {
-        f(ctxt.0, ctxt, &data);
+        f(ctxt.0, ctxt, &data.key());
     }
 }