Skip to content

Commit 5b2c536

Browse files
committed
Cached FQN on ClassInfo
1 parent 8ae35c6 commit 5b2c536

12 files changed

Lines changed: 144 additions & 21 deletions

File tree

docs/todo/eager-resolution.md

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -204,25 +204,36 @@ the resolution pipeline.
204204
drop + associated malloc/free). Combined with the double-walk
205205
fix, should bring examples/demo.php from ~9.5s to ~5-6s.
206206

207-
#### Phase 4d: Reduce string formatting overhead
208-
209-
**Impact: Medium. Effort: Low-Medium.**
210-
211-
3% self-time in `core::fmt::write` + 1.4% in `format_inner` +
212-
1.3% in `Ustr::from` (atom interning). These come from:
213-
214-
- `format!("{}\\{}", ns, name)` in `ClassInfo::fqn()` — called
215-
on every resolution. Already mitigated by returning `Atom` but
216-
the initial intern still allocates.
217-
- Type string construction during template substitution.
218-
- `name.to_string()` calls throughout the resolution pipeline.
219-
220-
Plan: audit hot-path `format!()` calls and replace with
221-
pre-computed or cached values where possible.
222-
223-
### Profiling data (current, release build, examples/demo.php)
224-
225-
Measured after Phase 4a + double-walk fix:
207+
#### Phase 4d: Cache the FQN on ClassInfo ✓
208+
209+
Added a cached `fqn: Option<Atom>` field to `ClassInfo`. Every
210+
hot-path `.fqn()` call previously ran `format!("{}\\{}", ns, name)`
211+
plus an `Ustr::from` intern lookup for namespaced classes; the
212+
resulting `String` allocation/free showed up across the
213+
`malloc`/`memmove`/`free` self-time. `name` and `file_namespace` are
214+
only ever set at the two parse/index-load stamping sites
215+
(`parser/ast_update.rs`, `resolution.rs`) and never mutated
216+
afterwards, so `cache_fqn()` is called there once the namespace is
217+
final and the cache can never go stale. `fqn()` returns the cached
218+
value when present and falls back to computing it on demand for the
219+
handful of synthetic classes that never pass through a stamping site
220+
(`__object_shape`, `stdClass`, etc.), which behave exactly as before.
221+
Excluded from `signature_eq` since it is derived from `name` +
222+
`file_namespace`, both already compared there.
223+
224+
Measured: `analyze examples/demo.php` dropped from ~1.75s to ~1.59s
225+
(release build), memory flat.
226+
227+
The remaining string-formatting costs are structural rather than
228+
incidental and are split out into Phase 4g below.
229+
230+
### Profiling data (release build, examples/demo.php)
231+
232+
`analyze examples/demo.php` currently completes in **~1.6s** (release
233+
build) after Phases 4a–4d landed. The historical breakdown below was
234+
measured after Phase 4a + the double-walk fix, before the Phase 4c
235+
ClassInfo Arc-ification and the Phase 4d FQN cache eliminated most of
236+
the allocation churn that dominated it:
226237

227238
| Phase | Wall time | Notes |
228239
|---|---|---|
@@ -232,13 +243,13 @@ Measured after Phase 4a + double-walk fix:
232243
| Slow diagnostics | 0.6s | (was 9.4s before double-walk fix) |
233244
| **Total** | **~9.5s** | target: < 3s |
234245

235-
Instrumentation data from the scope walk:
246+
Instrumentation data from that scope walk:
236247

237248
- 8,726 calls to `resolve_rhs_expression` (8.3s top-level time)
238249
- 5,559 resolved-class cache hits, 396 misses
239250
- Subject pipeline fallback: 1 call (negligible)
240251

241-
The 8.3s is dominated by allocation churn (ClassInfo cloning)
252+
The 8.3s was dominated by allocation churn (ClassInfo cloning)
242253
inside the type resolution pipeline, not by resolution logic.
243254

244255
---
@@ -338,6 +349,30 @@ The problem is needing them for every runtime analysis thread.
338349
- No stack overflows on the full test suite or the largest available
339350
project.
340351

352+
#### Phase 4g: Carry `Atom` in `PhpType::Named`
353+
354+
**Impact: Low-Medium. Effort: Medium.**
355+
356+
Deferred from Phase 4d. With the FQN now cached, the residual
357+
per-substitution string cost is `PhpType::Named(cls.fqn().to_string())`
358+
(and the equivalent patterns in `template_subs.rs`, `return_types.rs`,
359+
`inheritance/mod.rs`, `forward_walk/scope_state.rs`,
360+
`rhs_resolution/mod.rs`, and the Laravel builder providers). Each of
361+
these turns a cached `Atom` back into an owned `String` because
362+
`PhpType::Named` holds a `String`.
363+
364+
Changing `PhpType::Named` (and the analogous per-member cache keys such
365+
as `format!("{}::{}", class_fqn, method.name)` in `target_cache.rs` /
366+
`property_access.rs`) to carry `Atom` would let the resolution pipeline
367+
thread interned names end-to-end without re-allocating. This touches a
368+
core enum used throughout the type engine, so it is a standalone
369+
refactor rather than part of the 4d fqn-cache change.
370+
371+
**Success criteria:**
372+
- `PhpType::Named` carries an interned name (no `String`
373+
reallocation on the substitution hot path).
374+
- No test regressions.
375+
341376
---
342377

343378

src/code_actions/extract_interface.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -584,6 +584,7 @@ mod tests {
584584
backed_type: None,
585585
attribute_targets: 0,
586586
laravel: Default::default(),
587+
fqn: None,
587588
};
588589

589590
let src = generate_interface_source(&InterfaceGenParams {
@@ -644,6 +645,7 @@ mod tests {
644645
backed_type: None,
645646
attribute_targets: 0,
646647
laravel: Default::default(),
648+
fqn: None,
647649
};
648650

649651
let edit = build_implements_edit(content, &class, "UserInterface").unwrap();
@@ -692,6 +694,7 @@ mod tests {
692694
backed_type: None,
693695
attribute_targets: 0,
694696
laravel: Default::default(),
697+
fqn: None,
695698
};
696699

697700
let edit = build_implements_edit(content, &class, "UserInterface").unwrap();

src/completion/source/throws_analysis_tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,7 @@ fn make_class_with_throws(name: &str, methods: Vec<(&str, Vec<&str>)>) -> Arc<Cl
885885
backed_type: None,
886886
attribute_targets: 0,
887887
laravel: None,
888+
fqn: None,
888889
})
889890
}
890891

src/document_symbols.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,7 @@ mod tests {
695695
backed_type: None,
696696
attribute_targets: 0,
697697
laravel: None,
698+
fqn: None,
698699
};
699700
let detail = build_class_detail(&class);
700701
assert_eq!(detail, Some("extends Bar implements Baz, Qux".to_string()));
@@ -741,6 +742,7 @@ mod tests {
741742
backed_type: None,
742743
attribute_targets: 0,
743744
laravel: None,
745+
fqn: None,
744746
};
745747
let detail = build_class_detail(&class);
746748
assert_eq!(detail, Some("extends Bar".to_string()));

src/parser/anonymous.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ impl Backend {
112112
method_index: Default::default(),
113113
indexed_method_count: 0,
114114
laravel: None,
115+
fqn: None,
115116
}
116117
}
117118

src/parser/ast_update.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,7 @@ impl Backend {
586586
.map(|(c, ns)| {
587587
let mut cls = c.clone();
588588
cls.file_namespace = ns.as_deref().map(atom);
589+
cls.cache_fqn();
589590
cls
590591
})
591592
.collect();

src/parser/classes.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,7 @@ impl Backend {
297297
method_index: Default::default(),
298298
indexed_method_count: 0,
299299
laravel: Some(Box::new(laravel_metadata)),
300+
fqn: None,
300301
});
301302

302303
// Walk method bodies for anonymous classes.
@@ -404,6 +405,7 @@ impl Backend {
404405
method_index: Default::default(),
405406
indexed_method_count: 0,
406407
laravel: None,
408+
fqn: None,
407409
});
408410

409411
// Walk method bodies for anonymous classes.
@@ -489,6 +491,7 @@ impl Backend {
489491
method_index: Default::default(),
490492
indexed_method_count: 0,
491493
laravel: None,
494+
fqn: None,
492495
});
493496

494497
// Walk method bodies for anonymous classes.
@@ -636,6 +639,7 @@ impl Backend {
636639
method_index: Default::default(),
637640
indexed_method_count: 0,
638641
laravel: None,
642+
fqn: None,
639643
});
640644

641645
// Walk method bodies for anonymous classes.

src/resolution.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,7 @@ impl Backend {
659659
.or(file_namespace.as_deref())
660660
.map(crate::atom::atom);
661661
}
662+
cls.cache_fqn();
662663
}
663664

664665
// Apply class stub patches for phpstorm-stubs deficiencies

src/test_fixtures.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ pub fn make_class(name: &str) -> ClassInfo {
6868
backed_type: None,
6969
attribute_targets: 0,
7070
laravel: None,
71+
fqn: None,
7172
}
7273
}
7374

src/toposort.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ mod tests {
256256
backed_type: None,
257257
attribute_targets: 0,
258258
laravel: None,
259+
fqn: None,
259260
}
260261
}
261262

0 commit comments

Comments
 (0)