Skip to content

perform less decoding if it has the same syntax context #129827

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

Merged
merged 1 commit into from
Mar 29, 2025
Merged

perform less decoding if it has the same syntax context #129827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions compiler/rustc_metadata/src/rmeta/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>>>;

Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_middle/src/query/on_disk_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_serialize::opaque::{FileEncodeResult, FileEncoder, IntEncodedWithFixed
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
use rustc_session::Session;
use rustc_span::hygiene::{
ExpnId, HygieneDecodeContext, HygieneEncodeContext, SyntaxContext, SyntaxContextData,
ExpnId, HygieneDecodeContext, HygieneEncodeContext, SyntaxContext, SyntaxContextKey,
};
use rustc_span::source_map::Spanned;
use rustc_span::{
Expand Down Expand Up @@ -75,9 +75,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
Expand Down Expand Up @@ -305,7 +305,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(
Expand Down Expand Up @@ -566,7 +566,7 @@ impl<'a, 'tcx> SpanDecoder for CacheDecoder<'a, 'tcx> {
// 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
})
})
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/parameterized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
203 changes: 63 additions & 140 deletions compiler/rustc_span/src/hygiene.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -94,6 +91,21 @@ impl SyntaxContextData {
self.dollar_crate_name == kw::Empty
}

fn new(
(parent, outer_expn, outer_transparency): SyntaxContextKey,
opaque: SyntaxContext,
opaque_and_semitransparent: SyntaxContext,
) -> Self {
SyntaxContextData {
parent,
outer_expn,
outer_transparency,
opaque,
opaque_and_semitransparent,
dollar_crate_name: kw::DollarCrate,
}
}

fn key(&self) -> SyntaxContextKey {
(self.parent, self.outer_expn, self.outer_transparency)
}
Expand Down Expand Up @@ -574,67 +586,49 @@ impl HygieneData {

fn apply_mark_internal(
&mut self,
ctxt: SyntaxContext,
parent: SyntaxContext,
expn_id: ExpnId,
transparency: Transparency,
) -> SyntaxContext {
let syntax_context_data = &mut self.syntax_context_data;
debug_assert!(!syntax_context_data[ctxt.0 as usize].is_decode_placeholder());
let mut opaque = syntax_context_data[ctxt.0 as usize].opaque;
let mut opaque_and_semitransparent =
syntax_context_data[ctxt.0 as usize].opaque_and_semitransparent;

if transparency >= Transparency::Opaque {
let parent = opaque;
opaque = *self
.syntax_context_map
.entry((parent, expn_id, transparency))
.or_insert_with(|| {
let new_opaque = SyntaxContext::from_usize(syntax_context_data.len());
syntax_context_data.push(SyntaxContextData {
outer_expn: expn_id,
outer_transparency: transparency,
parent,
opaque: new_opaque,
opaque_and_semitransparent: new_opaque,
dollar_crate_name: kw::DollarCrate,
});
new_opaque
});
}

if transparency >= Transparency::SemiTransparent {
let parent = opaque_and_semitransparent;
opaque_and_semitransparent = *self
.syntax_context_map
.entry((parent, expn_id, transparency))
.or_insert_with(|| {
let new_opaque_and_semitransparent =
SyntaxContext::from_usize(syntax_context_data.len());
syntax_context_data.push(SyntaxContextData {
outer_expn: expn_id,
outer_transparency: transparency,
parent,
opaque,
opaque_and_semitransparent: new_opaque_and_semitransparent,
dollar_crate_name: kw::DollarCrate,
});
new_opaque_and_semitransparent
});
debug_assert!(!self.syntax_context_data[parent.0 as usize].is_decode_placeholder());
// Look into the cache first.
let key = (parent, expn_id, transparency);
if let Some(ctxt) = self.syntax_context_map.get(&key) {
return *ctxt;
}
// Reserve a new syntax context.
let ctxt = SyntaxContext::from_usize(self.syntax_context_data.len());
self.syntax_context_data.push(SyntaxContextData::decode_placeholder());
self.syntax_context_map.insert(key, ctxt);

// Opaque and semi-transparent versions of the parent. Note that they may be equal to the
// parent itself. E.g. `parent_opaque` == `parent` if the expn chain contains only opaques,
// and `parent_opaque_and_semitransparent` == `parent` if the expn contains only opaques
// and semi-transparents.
let parent_opaque = self.syntax_context_data[parent.0 as usize].opaque;
let parent_opaque_and_semitransparent =
self.syntax_context_data[parent.0 as usize].opaque_and_semitransparent;

// Evaluate opaque and semi-transparent versions of the new syntax context.
let (opaque, opaque_and_semitransparent) = match transparency {
Transparency::Transparent => (parent_opaque, parent_opaque_and_semitransparent),
Transparency::SemiTransparent => (
parent_opaque,
// Will be the same as `ctxt` if the expn chain contains only opaques and semi-transparents.
self.apply_mark_internal(parent_opaque_and_semitransparent, expn_id, transparency),
),
Transparency::Opaque => (
// Will be the same as `ctxt` if the expn chain contains only opaques.
self.apply_mark_internal(parent_opaque, expn_id, transparency),
// Will be the same as `ctxt` if the expn chain contains only opaques and semi-transparents.
self.apply_mark_internal(parent_opaque_and_semitransparent, expn_id, transparency),
),
};

let parent = ctxt;
*self.syntax_context_map.entry((parent, expn_id, transparency)).or_insert_with(|| {
syntax_context_data.push(SyntaxContextData {
outer_expn: expn_id,
outer_transparency: transparency,
parent,
opaque,
opaque_and_semitransparent,
dollar_crate_name: kw::DollarCrate,
});
SyntaxContext::from_usize(syntax_context_data.len() - 1)
})
// Fill the full data, now that we have it.
self.syntax_context_data[ctxt.as_u32() as usize] =
SyntaxContextData::new(key, opaque, opaque_and_semitransparent);
ctxt
}
}

Expand Down Expand Up @@ -1265,7 +1259,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
Expand Down Expand Up @@ -1323,9 +1317,6 @@ struct HygieneDecodeContextInner {
/// 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.
Expand Down Expand Up @@ -1396,10 +1387,10 @@ pub fn decode_expn_id(
// 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>(
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 {
Expand All @@ -1408,58 +1399,9 @@ 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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this cache was probably a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember that the mechanism here was designed to prevent redundant locking, though I'm not entirely sure if it relates to performance. Let's wait for the performance test results to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

#139228 showed almost the same regressions as #129827 (comment).

@bvanjoi could you resubmit this PR with remapped_ctxts caching restored and #129827 (comment) addressed?

// This has already been decoded.
return ctxt;
}

match inner.decoding.entry(raw_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The decoding field can also be removed now.

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_key = decode_ctxt_key(d, raw_id);

let ctxt = HygieneData::with(|hygiene_data| {
match hygiene_data.syntax_context_map.get(&ctxt_key) {
Expand All @@ -1473,29 +1415,10 @@ pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContext
Some(&ctxt) => ctxt,
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax_context_map lookup is no longer necessary, alloc_ctxt/apply_mark_internal already performs it.

// 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
}
None => hygiene_data.apply_mark_internal(ctxt_key.0, ctxt_key.1, ctxt_key.2),
}
});

// 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 {
Expand All @@ -1507,15 +1430,15 @@ pub fn decode_syntax_context<D: Decoder, F: FnOnce(&mut D, u32) -> SyntaxContext
ctxt
}

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());
}
}

Expand Down
Loading
Loading