Improve diagnostics multi threading

Author: AJenbo

docs/CHANGELOG.md

@@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Lower memory use for member access spans.** The subject text recorded for every `->`/`::` access (e.g. `$this` in `$this->save()`) no longer allocates a string when it is a plain slice of the source, which covers the vast majority of accesses in typical PHP code. It now reuses the file's own bytes instead, with an allocation only for the rarer cases (chained calls, `new` expressions) where the recorded text differs from the source.
 - **Faster diagnostics on large projects.** Several diagnostic checks (by-reference parameter detection, the `Stringable`-to-`string` acceptance check, and `model-property<Model>` literal validation) now read a class's inheritance from the resolved-class cache instead of re-merging traits, parent classes, and generics from scratch on every call. On large Laravel projects this removed a measurable share of the diagnostic pass's CPU time, and as a side effect these checks now also see interface-declared members (e.g. a `__toString` declared only on an implemented interface).
 - **Laravel string keys are collected once instead of once per CPU core.** Checking a `route()`, `config()`, `view()`, or `__()` key needs the project's full list of valid keys, and building each list walks the project directory. The diagnostic pass runs one worker per core, so all of them used to reach the empty list at the same moment and every one repeated the same walk, leaving most cores waiting on the disk rather than analysing. Each list is now built once and shared, which roughly doubles the number of cores the diagnostic pass keeps busy and cuts whole-project analysis time by around 15% on large Laravel projects.
+- **Repeated class lookups are remembered instead of searched again.** Answering "which class does this name refer to?" is the single most frequent thing PHPantom does: analysing a large Laravel project asks it millions of times, over only a few thousand distinct classes. Every question used to hash the name and take a read lock on two shared indexes, so with one worker per CPU core the workers spent much of their time queueing behind each other instead of analysing. Each worker now remembers the answers it has already looked up and drops them the moment the class indexes change, so a repeated question costs a single pointer comparison. Whole-project analysis is 8-12% faster on large Laravel projects and uses up to a third less CPU, with identical results and no measurable change in memory use. Hover, completion, and go-to-definition resolve names through the same path and get the same saving.
 
 ### Removed
 

docs/todo/performance.md

@@ -895,7 +895,7 @@ current single populate thread.
 
 Measured on a 32-core machine against large Laravel projects (release
 build): the `analyze` Phase 2 diagnostic pass spawns one worker per
-core with atomic work-stealing but does not keep them busy. Two
+core with atomic work-stealing but does not keep them busy. Three
 offenders are fixed. The original one was every worker deep-cloning
 the embedded stub class/function/constant indexes twice per file via
 `clone_for_diagnostic_worker`; the indexes are Arc-shared now. The
@@ -909,45 +909,126 @@ gitignore-aware walk. They are guarded by
 Laravel project from 11.6 to 22.6 of 32 cores (1.91 s → 1.31 s) and
 whole-run wall clock down ~15%.
 
+The third was the name → class loader itself. `find_or_load_class_typed`
+was called 3.68 M times in a 1.2 s Phase 2 over a few thousand distinct
+types, and every call hashed the name case-insensitively for a read
+lock on `class_not_found_cache` and another on `fqn_class_index`. That
+cluster (`find_or_load_class_typed`, `find_class_in_uri_classes_index`,
+`CiMap::get`, `sip::Hasher::write`, `RawRwLock::lock_shared_slow`,
+kernel `osq_lock`) was ~22% of Phase 2 samples, and both lock symbols
+have since dropped out of the profile entirely. `class_loader_memo`
+memoises the loader per worker on the interned `PhpType` handle, keyed
+by `SymbolIndex::id` and stamped with `class_lookup_generation` so an
+answer is never staler than the two caches it derives from. Phase 2 fell
+~25-32% (1.23 s → 0.93 s and 0.72 s → 0.49 s on the two largest Laravel
+projects benchmarked) at ~26.8 → ~28.6 of 32 cores, with whole-run wall
+clock down 8-12% and user CPU down 17-34% across three large Laravel
+projects. Every smaller project benchmarked improved as well, RSS did
+not move, and diagnostic output was byte-identical on all ten.
+
 Sampling `/proc/<pid>/task/*/stat` is the fastest way to see the
 remaining ceiling: workers in `S` rather than `R` are blocked, not
-computing. After the stampede fix the remaining gap is:
-
-- ~18% of worker on-CPU time is malloc/free/memmove, diffuse:
-  `PhpType`, `MethodInfo`, and `ParameterInfo` clones throughout the
-  type engine, plus allocator arena contention when 32 workers
-  allocate heavily.
-- The `PhpType` interner (`php_type::intern`) is hit ~13 M times in a
-  1.3 s Phase 2, roughly a quarter of which fall through the shard
-  read lock to the write path. Its 64 shards are the largest single
-  source of measured lock-wait time left (~5 s of thread-time across
-  all workers), and `sched_yield` volume shows the workers spinning
-  before parking.
+computing. What is left, re-profiled after the memo:
+
+- Type strings are still parsed during the diagnostic pass rather than
+  at index time: `TypeTokenStream::fill_buffer_slow`, `PhpType::parse`,
+  `LocalArena::alloc_slice_copy` and `parse_primary_type` together are
+  ~6.5% of Phase 2 samples. This is P14's territory.
+- malloc/free/memmove remains diffuse. `is_scalar_name` and
+  `is_keyword_type` (~2.3% between them) allocate a lowercase `String`
+  per call and are reached from `base_name`, the subtype checks and the
+  narrowing paths; the ~150 other `to_ascii_lowercase()` calls in
+  `php_type/` do the same. A stack buffer plus a length pre-filter
+  would make them allocation-free, but see the reverted attempt below
+  before assuming that wins.
+- The `PhpType` interner (`php_type::intern` + `intern::lookup`) is
+  ~4.6% of Phase 2 samples and its 64 shards are now the largest
+  remaining lock, though the memo removed enough traffic that
+  `lock_shared_slow` no longer registers at all. An earlier count put
+  the interner at ~13 M hits per pass with roughly a quarter falling
+  through the shard read lock to the write path; that has not been
+  re-counted since. A per-thread direct-mapped memo in front of the
+  shard read (the same shape as `class_loader_memo`) is the obvious
+  next attempt.
 - `ensure_workspace_indexed_with_progress` still re-walks the whole
   workspace on every call, so the four surviving string-key
   enumerations do four full walks per diagnostic pass (down from ~44).
   The walk is deliberate — it is how PHP files created outside the
   editor get discovered — so removing it needs a change to that
   contract, not just another cache.
-- Projects that keep substantial code in `vendor/` (e.g. models shipped
-  in a shared vendor package) pay extra: eager population only walks
-  classes from the indexed user files, so every vendor class is parsed
-  lazily on first touch during the diagnostic pass, serialising workers
-  on the load path early in the run. Measured on a large Laravel
-  project (release build, 32 threads): eager population covers only
-  ~1,300 user classes, and the diagnostic pass then resolves ~2,100
-  further classes lazily, with cycle-break re-merges that eager
-  dependency-first ordering would have avoided (eager population
-  itself hit only 3).
-
-Likely next steps, in rough value order: reduce clone traffic in the
-hot type-engine paths (share via `Arc`/`Cow` instead of cloning
-`PhpType`/member vectors), cut interner traffic or widen its sharding,
-include depended-upon vendor classes in eager population, and only
-then consider allocator-level fixes. The LSP workspace diagnostics
-pass uses the same collectors and has the same ceiling. Re-measure
-with `perf` (frame-pointer build) or the CPU-sampling loop in the
-Appendix after any change.
+- `class_loader_memo`'s own hit rate is bounded by how often
+  `note_class_lookup_change` fires: 1,169 times in Phase 2, once per
+  lazily parsed vendor file, each retiring every worker's table. A
+  build with invalidation removed (unsound, for sizing only) reached
+  0.80 s against the 0.93 s shipped, so ~14% of the memo's prize is
+  still on the table. Recovering it means either loading fewer vendor
+  classes lazily (the reverted experiment below, whose calculus this
+  changes) or splitting the generation so an additive insert only
+  retires negative answers. The latter is not sound as stated: a
+  positive answer reached through PSR-4 or a stub is matched by short
+  name, so it is not always backed by an `fqn_class_index` entry under
+  the name that was looked up, and a later first-time insert of that
+  exact name can change it.
+
+The LSP workspace diagnostics pass uses the same collectors and has the
+same ceiling. Re-measure with `perf` (frame-pointer build) or the
+CPU-sampling loop in the Appendix after any change.
+
+**Tried and reverted (vendor classes in eager population):** an
+earlier revision of this item claimed that projects keeping
+substantial code in `vendor/` pay extra because eager population only
+walks the indexed user files, leaving every vendor class to be parsed
+and resolved lazily mid-diagnostic — serialising workers on the load
+path, with cycle-break re-merges that dependency-first ordering would
+have avoided. Seeding eager population with vendor classes was
+implemented in two escalating variants and benchmarked (10-run,
+order-swapped wall/user-CPU averages against the two largest Laravel
+projects benchmarked): (1) expanding the toposort input with the
+transitive inheritance closure of the user classes (parents, traits,
+interfaces, mixins, generic arguments, loaded via
+`find_or_load_class`), and (2) additionally seeding every `use`-import
+target found in `fqn_uri_index`, with parallel frontier loading and
+Kahn-levelled parallel resolution to keep Phase 1.5 off the critical
+path. Neither moved wall clock on any project. Variant 1 cut lazy
+Phase 2 resolutions only ~2% (950 → 931) because vendor ancestors
+were already being resolved as nested resolutions during eager
+population; variant 2 cut them to a third (931 → 276) but cost ~3%
+more user CPU (duplicated nested provider resolutions across level
+workers) and ~10 MB RSS for classes the pass never needed resolved.
+The predicted cycle-break re-merges also failed to reproduce: Phase 2
+hits 0 on one of them and 18 on the other, and all
+18 are genuine dependency cycles (`Schedule` ↔
+`PendingEventAttributes`, Spatie `Role` ↔ `Permission`) that
+dependency-first ordering cannot avoid — the toposort has to break
+them somewhere too. Conclusion: after the stampede fix, lazy vendor
+class loading no longer serialises the diagnostic pass measurably;
+the remaining ceiling is the clone/interner traffic above. Diagnostic
+output was byte-identical in every configuration.
+
+**Tried and reverted:** a single `perf` snapshot attributed ~4% of
+Phase 2 time to `core::hash::sip::Hasher::write`, called from
+`CiMap`/`CiSet` (`fqn_class_index`, `class_not_found_cache`) inside
+`find_or_load_class` — the single hottest function in the profile.
+Two independent fixes were tried: swapping `CiMap`/`CiSet`'s `HashMap`
+from `std`'s default SipHash to a hand-rolled FxHash-style hasher, and
+avoiding `fold()`'s per-lookup heap allocation with a stack buffer.
+Both looked sound in isolation and passed all tests, but 10-run
+wall-clock/user-CPU averages against a large Laravel project (order-swapped
+to rule out warm-cache bias) showed a small, consistent *regression*
+(~2% more user CPU) for the hasher swap alone, the allocation-avoidance
+alone, and the two combined. Likely cause: `mimalloc` already makes
+these transient allocations cheap, and the hand-rolled hasher's
+sequential dependency chain (`rotate_left` → `xor` → `wrapping_mul` per
+word) didn't beat `std`'s SipHash13 for these short keys on this
+hardware. Lesson for next attempt: a single `perf --stdio` percentage
+is not sufficient evidence — confirm with repeated, order-controlled
+wall-clock measurement before committing a "hot function" fix; sampling
+noise and inlining attribution can point at the wrong function. The
+re-measurement did surface a more promising lead: `RawRwLock::lock_shared_slow`
+rose from 3.78% to 4.36% of samples once the hashing/allocation cost
+was removed, suggesting the read lock on `fqn_class_index` (contended
+by 32 workers doing `find_or_load_class` concurrently) is closer to the
+real ceiling than the hashing was.
 
 ---
 

src/class_loader_memo.rs

@@ -0,0 +1,146 @@
+//! Per-thread memo for [`Backend::find_or_load_class_typed`].
+//!
+//! [`Backend`]: crate::Backend
+//!
+//! The diagnostic pass asks for the same classes over and over: one
+//! `analyze` run on a large Laravel project makes ~3.7 M name → class
+//! lookups over a few thousand distinct types. Every one of them walked the
+//! multi-phase loader in `resolution.rs`, hashing the name
+//! case-insensitively for a read lock on `class_not_found_cache` and
+//! another on `fqn_class_index`. With one worker per core those two lock
+//! words were the pass's ceiling: the loader and the lock and hash code
+//! under it were ~22% of its samples, ~7% of that spent purely acquiring
+//! the read locks.
+//!
+//! Interned [`PhpType`] handles are a far cheaper key than the name: one
+//! multiply to index a slot, then pointer equality. A repeat lookup
+//! touches nothing but this thread's own table.
+//!
+//! # Freshness
+//!
+//! A slot is served only while its stamp matches
+//! [`SymbolIndex::class_lookup_generation`], which
+//! [`SymbolIndex::note_class_lookup_change`] bumps on every mutation of
+//! `fqn_class_index` and every clear of `class_not_found_cache`. Those are
+//! the two structures a loader answer is derived from, so a memoised
+//! answer is never staler than the caches it came from. Retired slots are
+//! overwritten in place rather than swept, so invalidation costs nothing.
+//!
+//! [`SymbolIndex::class_lookup_generation`]: crate::symbol_index::SymbolIndex::class_lookup_generation
+//! [`SymbolIndex::note_class_lookup_change`]: crate::symbol_index::SymbolIndex::note_class_lookup_change
+//!
+//! # Why a fixed table
+//!
+//! Memory is bounded with no eviction policy to tune: `SLOTS` entries per
+//! thread that ever looks a class up, holding one type handle and one
+//! `Arc<ClassInfo>` each — both already kept alive by the interner and the
+//! class index. A long-lived server cannot grow it, and a collision costs
+//! one recomputation.
+//!
+//! # Why per-thread
+//!
+//! A shared table would reintroduce the lock this memo exists to avoid.
+//! Threads are not the unit of ownership, though, so each slot records
+//! which [`SymbolIndex`] produced it: a thread reused across `Backend`s
+//! (nextest runs many in one process) cannot be served another project's
+//! classes.
+//!
+//! [`SymbolIndex`]: crate::symbol_index::SymbolIndex
+
+use std::cell::RefCell;
+use std::sync::Arc;
+
+use crate::php_type::PhpType;
+use crate::types::ClassInfo;
+
+/// Slots per thread, a power of two so the index is a mask.
+///
+/// 8 KB per thread at 32 bytes a slot. Measured against 1024 on two large
+/// Laravel projects: the smaller table won slightly on both, so the working
+/// set of one file's diagnostics fits and the locality of a table that
+/// stays in L1 is worth more than the collisions avoided by a larger one.
+const SLOTS: usize = 256;
+
+/// One memoised answer.
+struct Slot {
+    /// The type this answer is for, or `None` in a slot never written.
+    ///
+    /// Held as a handle rather than as the address it hashes to: dropping
+    /// the last handle to a type frees the interned node, and a later type
+    /// could be allocated at the same address and match a stale key.
+    key: Option<PhpType>,
+    /// The loader's answer, itself `None` for "no such class".
+    class: Option<Arc<ClassInfo>>,
+    /// Identity of the index that produced `class`.
+    owner: u64,
+    /// Index generation `class` was looked up at.
+    generation: u64,
+}
+
+impl Slot {
+    const fn empty() -> Slot {
+        Slot {
+            key: None,
+            class: None,
+            owner: 0,
+            generation: 0,
+        }
+    }
+}
+
+thread_local! {
+    /// Allocated on the first lookup, so threads that never resolve a
+    /// class pay nothing for it.
+    static TABLE: RefCell<Option<Box<[Slot]>>> = const { RefCell::new(None) };
+}
+
+/// Slot for `ty`: Fibonacci hashing of the node address, whose low bits
+/// are always zero from the allocator's alignment.
+#[inline]
+fn slot_of(ty: &PhpType) -> usize {
+    const PHI: u64 = 0x9e37_79b9_7f4a_7c15;
+    ((ty.identity() as u64).wrapping_mul(PHI) >> (64 - SLOTS.trailing_zeros())) as usize
+}
+
+/// The memoised answer for `ty`, or `None` when there is none and the
+/// caller must run the loader and report back to [`store`].
+///
+/// The outer `Option` distinguishes "not memoised" from a memoised
+/// "no such class"; both are worth caching, since a negative answer costs
+/// the same multi-phase walk as a positive one.
+pub(crate) fn probe(owner: u64, generation: u64, ty: &PhpType) -> Option<Option<Arc<ClassInfo>>> {
+    TABLE.with(|table| {
+        let table = table.borrow();
+        let slot = &table.as_ref()?[slot_of(ty)];
+        if slot.owner != owner || slot.generation != generation {
+            return None;
+        }
+        // Pointer equality: a different type hashing to this slot is a
+        // miss, not a wrong answer.
+        (slot.key.as_ref()? == ty).then(|| slot.class.clone())
+    })
+}
+
+/// Record `class` as the answer for `ty`, replacing whatever occupied the
+/// slot.
+///
+/// Called only after [`probe`] has returned, never with the loader still
+/// running: loading a class parses a file, which resolves further classes
+/// and re-enters this table.
+pub(crate) fn store(owner: u64, generation: u64, ty: &PhpType, class: &Option<Arc<ClassInfo>>) {
+    TABLE.with(|table| {
+        let mut table = table.borrow_mut();
+        let table = table
+            .get_or_insert_with(|| (0..SLOTS).map(|_| Slot::empty()).collect::<Vec<_>>().into());
+        table[slot_of(ty)] = Slot {
+            key: Some(ty.clone()),
+            class: class.clone(),
+            owner,
+            generation,
+        };
+    });
+}
+
+#[cfg(test)]
+#[path = "class_loader_memo_tests.rs"]
+mod tests;