nandi/rustnim · 51929ac

⑂ 51929ac

Publish `Self` apart from the const-name map

Putting the current `Self` in that map's cache key undid the caching entirely:
`self_ty` changes with every impl, so the key changed on nearly every call and
the whole map was rebuilt each time. A hundred crates went back from forty
seconds to over twenty minutes -- the exact cost the cache was added to remove,
reintroduced by the commit after it.

`Self` is a single string, so it is published on its own and substituted at
lookup. The map stays keyed on real type names and rebuilds only when the
tables it reads have grown.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandithebull committed 2026-09-19T12:41:16-07:00 Browse files

51929ac parent: 8e6b002

modified src/lower.rs +10 -10 Viewed

@@ -348,7 +348,7 @@ pub struct Lowerer {
348	348	/// parameter is the same thing: a generic parameter that is a value.
349	349	const_gens: HashMap<String, String>,
350	350	/// Combined table size when `CONST_NAMES` was last rebuilt.
351		- const_names_at: std::cell::RefCell<Option<(usize, String)>>,
	351	+ const_names_at: std::cell::Cell<Option<usize>>,
352	352	/// Bodies of functions whose return type is a return-position `impl
353	353	/// Trait`, keyed the same way as `fns`/`methods`, so `resolve_opaque` can
354	354	/// read the concrete type back off the body.
@@ -437,7 +437,7 @@ impl Lowerer {
437	437	emitted_local: std::collections::HashSet::new(),
438	438	emitted_assoc: std::collections::HashSet::new(),
439	439	const_gens: HashMap::new(),
440		- const_names_at: std::cell::RefCell::new(None),
	440	+ const_names_at: std::cell::Cell::new(None),
441	441	opaque_fns: Vec::new(),
442	442	opaque_ret: HashMap::new(),
443	443	opaque_hint: None,
@@ -1289,14 +1289,17 @@ impl Lowerer {
1289	1289	// their combined size is enough to tell whether anything has been
1290	1290	// added since the last refill -- without it, a 357-crate run spends
1291	1291	// essentially all of its time rebuilding this map.
1292		- // `Self::CONST` resolves against whichever impl is being walked, so
1293		- // the current `Self` is part of what the map depends on.
1294		- let me = self.self_ty.as_ref().map(type_name).unwrap_or_default();
	1292	+ // `Self` is published separately: it changes with every impl, and
	1293	+ // making it part of this map's key rebuilt the map on nearly every
	1294	+ // call, which is the cost this cache exists to avoid.
	1295	+ ty::CURRENT_SELF.with(\|c\| {
	1296	+ *c.borrow_mut() = self.self_ty.as_ref().map(type_name).unwrap_or_default()
	1297	+ });
1295	1298	let size = self.enums.len() + self.assoc_consts.len();
1296		- if self.const_names_at.borrow().as_ref() == Some(&(size, me.clone())) {
	1299	+ if self.const_names_at.get() == Some(size) {
1297	1300	return;
1298	1301	}
1299		- *self.const_names_at.borrow_mut() = Some((size, me.clone()));
	1302	+ self.const_names_at.set(Some(size));
1300	1303	ty::CONST_NAMES.with(\|m\| {
1301	1304	let mut m = m.borrow_mut();
1302	1305	m.clear();
@@ -1310,9 +1313,6 @@ impl Lowerer {
1310	1313	}
1311	1314	for ((owner, name), (nim, _)) in &self.assoc_consts {
1312	1315	m.insert(format!("{owner}::{name}"), nim.clone());
1313		- if !me.is_empty() && owner == &me {
1314		- m.insert(format!("Self::{name}"), nim.clone());
1315		- }
1316	1316	}
1317	1317	});
1318	1318	}

      /// parameter is the same thing: a generic parameter that is a value.
      const_gens: HashMap<String, String>,
      /// Combined table size when `CONST_NAMES` was last rebuilt.
--    const_names_at: std::cell::RefCell<Option<(usize, String)>>,
++    const_names_at: std::cell::Cell<Option<usize>>,
      /// Bodies of functions whose return type is a return-position `impl
      /// Trait`, keyed the same way as `fns`/`methods`, so `resolve_opaque` can
      /// read the concrete type back off the body.
              emitted_local: std::collections::HashSet::new(),
              emitted_assoc: std::collections::HashSet::new(),
              const_gens: HashMap::new(),
--            const_names_at: std::cell::RefCell::new(None),
++            const_names_at: std::cell::Cell::new(None),
              opaque_fns: Vec::new(),
              opaque_ret: HashMap::new(),
              opaque_hint: None,
          // their combined size is enough to tell whether anything has been
          // added since the last refill -- without it, a 357-crate run spends
          // essentially all of its time rebuilding this map.
--        // `Self::CONST` resolves against whichever impl is being walked, so
++        // `Self` is published separately: it changes with every impl, and
--        // the current `Self` is part of what the map depends on.
++        // making it part of this map's key rebuilt the map on nearly every
--        let me = self.self_ty.as_ref().map(type_name).unwrap_or_default();
++        // call, which is the cost this cache exists to avoid.
++        ty::CURRENT_SELF.with(|c| {
++            *c.borrow_mut() = self.self_ty.as_ref().map(type_name).unwrap_or_default()
++        });
          let size = self.enums.len() + self.assoc_consts.len();
--        if self.const_names_at.borrow().as_ref() == Some(&(size, me.clone())) {
++        if self.const_names_at.get() == Some(size) {
              return;
          }
--        *self.const_names_at.borrow_mut() = Some((size, me.clone()));
++        self.const_names_at.set(Some(size));
          ty::CONST_NAMES.with(|m| {
              let mut m = m.borrow_mut();
              m.clear();
              }
              for ((owner, name), (nim, _)) in &self.assoc_consts {
                  m.insert(format!("{owner}::{name}"), nim.clone());
--                if !me.is_empty() && owner == &me {
--                    m.insert(format!("Self::{name}"), nim.clone());
--                }
              }
          });
      }

modified src/ty.rs +17 -0 Viewed

@@ -546,7 +546,24 @@ thread_local! {
546	546	std::cell::RefCell::new(std::collections::HashMap::new());
547	547	}
548	548
	549	+thread_local! {
	550	+ /// The type `Self` currently names. Kept apart from `CONST_NAMES` on
	551	+ /// purpose: it changes with every impl, and folding it into that map
	552	+ /// would rebuild the whole thing on nearly every call.
	553	+ pub static CURRENT_SELF: std::cell::RefCell<String> =
	554	+ const { std::cell::RefCell::new(String::new()) };
	555	+}
	556	+
549	557	fn known_const(q: &str, n: &str) -> Option<String> {
	558	+ let q = if q == "Self" {
	559	+ let me = CURRENT_SELF.with(\|c\| c.borrow().clone());
	560	+ if me.is_empty() {
	561	+ return None;
	562	+ }
	563	+ me
	564	+ } else {
	565	+ q.to_string()
	566	+ };
550	567	CONST_NAMES.with(\|m\| m.borrow().get(&format!("{q}::{n}")).cloned())
551	568	}
552	569

          std::cell::RefCell::new(std::collections::HashMap::new());
  }
++thread_local! {
++    /// The type `Self` currently names. Kept apart from `CONST_NAMES` on
++    /// purpose: it changes with every impl, and folding it into that map
++    /// would rebuild the whole thing on nearly every call.
++    pub static CURRENT_SELF: std::cell::RefCell<String> =
++        const { std::cell::RefCell::new(String::new()) };
++}
++
  fn known_const(q: &str, n: &str) -> Option<String> {
++    let q = if q == "Self" {
++        let me = CURRENT_SELF.with(|c| c.borrow().clone());
++        if me.is_empty() {
++            return None;
++        }
++        me
++    } else {
++        q.to_string()
++    };
      CONST_NAMES.with(|m| m.borrow().get(&format!("{q}::{n}")).cloned())
  }