Lower memory use for member access spans

Author: AJenbo

docs/CHANGELOG.md

@@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Updated the bundled mago toolchain to 1.43.0.** The parser, docblock parser, formatter, and supporting crates are refreshed to the latest upstream release. Contributed by @enwi in https://github.com/PHPantom-dev/phpantom_lsp/pull/234.
 - **Lower memory use in the cross-file reference index.** The index backing Find References and the reference-count inlay hints now keeps only the distinct files and counts each symbol actually needs, instead of one entry per matching location plus never-read position data. On large projects this removes millions of short-lived allocations and shrinks the index to a fraction of its previous size, with no change to Find References or inlay hint results.
 - **Lower memory use for method lookups.** Each resolved class's method name index is now a sorted list searched with binary search instead of a hash map, using about a third of the memory for the same lookup speed. On large Laravel projects, where the resolved-class cache holds thousands of these indexes, this measurably shrinks total memory use.
+- **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.
 
 ### Removed
 

docs/todo.md

@@ -28,7 +28,6 @@ within the same impact tier.
 | P36 | [Diagnostic-path call sites re-merge inheritance per call](todo/performance.md#p36-diagnostic-path-call-sites-re-merge-inheritance-per-call-instead-of-reading-the-resolved-class-cache)          | Medium      | Low         |
 | P38 | [Resolved type values are duplicated ~34x](todo/performance.md#p38-resolved-type-values-are-duplicated-34x)                                                          | High        | High        |
 | P41 | [3.8 M live allocations cost ~170 MB in allocator overhead](todo/performance.md#p41-38-m-live-allocations-cost-170-mb-in-allocator-overhead)                          | Medium      | Medium      |
-| P43 | [`MemberAccess.subject_text` still allocates a `String` per span](todo/performance.md#p43-memberaccesssubject_text-still-allocates-a-string-per-span) | Low-Medium  | Medium      |
 | P33 | [Workspace diagnostics leaves the whole project fully resolved in memory](todo/performance.md#p33-workspace-diagnostics-leaves-the-whole-project-fully-resolved-in-memory) | High        | Medium-High |
 | X10 | [Interactive requests block on the workspace index lock during initial indexing](todo/indexing.md#x10-interactive-requests-block-on-the-workspace-index-lock-during-initial-indexing) | Medium      | Medium      |
 | L21 | [Tighten the supertype-where-subtype comparison escape hatch (blocked on resolver precision)](todo/laravel.md#l21-tighten-the-supertype-where-subtype-comparison-escape-hatch-blocked-on-resolver-precision) | Medium      | High        |
@@ -115,6 +114,7 @@ unlikely to move the needle for most users.
 | D15 | [Unused parameter diagnostic](todo/diagnostics.md#d15-unused-parameter-diagnostic)                                                                                          | Low         | Low         |
 |     | **[Bug Fixes](todo/bugs.md)**                                                                                                                                               |             |             |
 | B1  | [Rename can build edits from a symbol map that predates the buffer](todo/bugs.md#b1-rename-can-build-edits-from-a-symbol-map-that-predates-the-buffer)                       | Medium-High | Low-Medium  |
+| B2  | [`mem-audit` feature build fails: `Ustr::capacity()` does not exist](todo/bugs.md#b2-mem-audit-feature-build-fails-ustrcapacity-does-not-exist)                              | Low         | Low         |
 |     | **[Code Actions](todo/actions.md)**                                                                                                                                         |             |             |
 | A40 | [Generate method from call](todo/actions.md#a40-generate-method-from-call)                                                                                                  | Medium-High | Medium      |
 | A41 | [Create class from non-existing name](todo/actions.md#a41-create-class-from-non-existing-name)                                                                              | Medium      | Medium      |

docs/todo/bugs.md

@@ -41,3 +41,39 @@ Note that the cross-file paths need slightly more than linked editing
 did: they read maps for files other than the request URI, so each one
 must be checked against *that* file's content, not the buffer the
 request arrived on.
+
+## B2 `mem-audit` feature build fails: `Ustr::capacity()` does not exist
+
+`cargo build --features mem-audit` fails on `main` (reproduced before
+any local changes, via `git stash`):
+
+```
+error[E0599]: no method named `capacity` found for reference `&Ustr` in the current scope
+   --> src/mem_audit.rs:1269:27
+    |
+1269 |                 sym.add(k.capacity());
+    |                           ^^^^^^^^ method not found in `&Ustr`
+
+error[E0599]: no method named `capacity` found for reference `&Ustr` in the current scope
+   --> src/mem_audit.rs:1271:34
+    |
+1271 |                 member_idx.add(k.capacity());
+    |                                  ^^^^^^^^ method not found in `&Ustr`
+```
+
+`k` in both call sites is a `Ustr` (interned string handle from the
+`ustr` crate) used as a hash map key, and the audit code is calling
+`.capacity()` on it to account for heap bytes the way it does for
+`String`/`Box<str>` keys elsewhere in the same file. `Ustr` has no
+`capacity()` method — it is a pointer into a global intern table, not
+an owned allocation, so the right accounting is either to treat it as
+zero marginal heap cost (the string data is shared and already counted
+once wherever it's interned) or to look up whatever `ustr` exposes for
+the underlying byte length, not capacity.
+
+The current profile (dev, `cargo build --lib`) never builds this
+feature, so normal development and `cargo test` are unaffected. It only
+surfaces when actually running the memory-audit binary, which is
+presumably why it went unnoticed. Fix by correcting the two call sites
+in `src/mem_audit.rs` to match whatever the installed `ustr` version's
+API actually provides.

docs/todo/performance.md

@@ -1215,62 +1215,6 @@ headless mode rather than special-casing the `analyze` call site.
 
 ---
 
-## P43. `MemberAccess.subject_text` still allocates a `String` per span
-
-**Impact: Low-Medium · Effort: Medium**
-
-Every other name field on `SymbolKind` (`member_name`, class names,
-variable names, function names, constant names) is now `Atom`,
-eliminating their per-span allocation. `MemberAccess::subject_text` is
-the one field left as an owned `String`, and it is not a simple
-byte-range problem the way it first looked.
-
-`subject_text` is not a slice of the file's raw source — it is the
-*synthesized* canonical text `expr_to_subject_text` builds from the AST
-(`symbol_map/extraction/subject_text.rs`, `type_engine/subject_expr.rs`).
-It diverges from the raw bytes in ways consumers actually depend on:
-
-- `(new Foo($x))->bar()` lowers straight to `"Foo"`, dropping `new` and
-  the constructor arguments entirely — documented as intentional,
-  matching historical behaviour ("the constructed instance resolves
-  through its class").
-- Whitespace around `->`/`::` is normalised away (`$this -> foo` still
-  serialises to `"$this->foo"`); a raw source slice would keep the
-  spaces and break the byte-exact splitting `SubjectExpr::parse` does
-  (`split_last_arrow_raw` and friends have no whitespace tolerance).
-- Expressions the lowering can't represent collapse to `""`, and
-  several consumers key off `.is_empty()` as an explicit
-  "unresolvable" sentinel (e.g. `resolve_target_classes` callers). A
-  raw slice would hand them real, non-empty text instead and defeat
-  that check.
-
-Storing `(start, end)` into the file content and slicing at request
-time would reproduce none of this and would silently change subject
-resolution for the shapes above — exactly the class of silently-wrong
-result the project's no-diagnostic-suppression rule exists to prevent
-for diagnostics, and the same logic applies to any other resolution
-path.
-
-A safe version of the same idea is still possible, just bigger than a
-plain byte range:
-
-1. Add an enum (`SubjectText::Range { start, end } |
-   SubjectText::Owned(Box<str>)`), chosen at extraction time by
-   comparing the synthesized text against the raw byte range of the
-   object expression and only paying for `Owned` when they differ
-   (nested calls, `new`-as-subject, unsupported forms).
-2. Thread the file content through the ~15 call sites across `hover`,
-   `definition`, `references`, `diagnostics`, `code_actions`, and the
-   `symbol_map` test helpers that currently read `subject_text` as a
-   bare `&str`, since `SubjectText::as_str` needs it to resolve the
-   `Range` case.
-
-Worth re-measuring after: most `MemberAccess` subjects in typical PHP
-code are plain variables/`$this`/class names, which take the free
-`Range` path, so the `Owned` fallback should be rare in practice.
-
----
-
 # Remaining anti-pattern fixes
 
 Most remaining depth-cap issues were addressed by eager class

src/code_actions/import_class.rs

@@ -239,7 +239,7 @@ impl Backend {
                     subject_text,
                     is_static: true,
                     ..
-                } => subject_text.as_str(),
+                } => subject_text.as_str(content),
                 _ => continue,
             };
 
@@ -585,11 +585,12 @@ impl Backend {
                     subject_text,
                     is_static: true,
                     ..
-                } if !subject_text.starts_with('$')
-                    && !subject_text.contains('\\')
-                    && !is_class_keyword(subject_text) =>
+                } if {
+                    let s = subject_text.as_str(content);
+                    !s.starts_with('$') && !s.contains('\\') && !is_class_keyword(s)
+                } =>
                 {
-                    subject_text.as_str()
+                    subject_text.as_str(content)
                 }
                 _ => continue,
             };
@@ -794,11 +795,12 @@ impl Backend {
                     subject_text,
                     is_static: true,
                     ..
-                } if !subject_text.starts_with('$')
-                    && !subject_text.contains('\\')
-                    && !is_class_keyword(subject_text) =>
+                } if {
+                    let s = subject_text.as_str(content);
+                    !s.starts_with('$') && !s.contains('\\') && !is_class_keyword(s)
+                } =>
                 {
-                    (subject_text.as_str(), ClassRefContext::Other)
+                    (subject_text.as_str(content), ClassRefContext::Other)
                 }
                 _ => continue,
             };

src/code_actions/replace_deprecated.rs

@@ -143,7 +143,7 @@ impl Backend {
 
                     // Resolve the subject to find the method's replacement template.
                     let base_class = resolve_subject_to_class(
-                        subject_text,
+                        subject_text.as_str(content),
                         *is_static,
                         &file_ctx,
                         span.start,
@@ -203,7 +203,7 @@ impl Backend {
                         continue;
                     };
 
-                    let subject = Some(subject_text.trim().to_string());
+                    let subject = Some(subject_text.as_str(content).trim().to_string());
                     let replacement =
                         expand_template(&replacement_template, &args_text, subject.as_deref());
 

src/completion/handler/member_access.rs

@@ -465,7 +465,7 @@ impl Backend {
             };
             return Some(CompletionTarget {
                 access_kind,
-                subject: subject_text.clone(),
+                subject: subject_text.as_str(content).to_string(),
             });
         }
 

src/definition/member/mod.rs

@@ -699,13 +699,13 @@ impl Backend {
         let offset = position_to_offset(content, position);
 
         // Try the symbol map (primary path).
-        if let Some(result) = self.member_access_from_symbol_map(uri, offset) {
+        if let Some(result) = self.member_access_from_symbol_map(uri, content, offset) {
             return Some(result);
         }
         // Retry with offset − 1 for the end-of-token edge case (cursor
         // right after the last character of the member name).
         if offset > 0
-            && let Some(result) = self.member_access_from_symbol_map(uri, offset - 1)
+            && let Some(result) = self.member_access_from_symbol_map(uri, content, offset - 1)
         {
             return Some(result);
         }
@@ -718,6 +718,7 @@ impl Backend {
     fn member_access_from_symbol_map(
         &self,
         uri: &str,
+        content: &str,
         offset: u32,
     ) -> Option<(String, AccessKind)> {
         let maps = self.symbol_maps.read();
@@ -734,7 +735,7 @@ impl Backend {
                 } else {
                     AccessKind::Arrow
                 };
-                Some((subject_text.clone(), access_kind))
+                Some((subject_text.as_str(content).to_string(), access_kind))
             }
             _ => None,
         }

src/definition/resolve.rs

@@ -276,7 +276,7 @@ impl Backend {
                 };
                 let mctx = MemberDefinitionCtx {
                     member_name,
-                    subject: subject_text,
+                    subject: subject_text.as_str(content),
                     access_kind,
                     access_hint,
                 };

src/definition/type_definition.rs

@@ -103,7 +103,7 @@ impl Backend {
 
                 let candidates = ResolvedType::into_arced_classes(
                     crate::type_engine::resolver::resolve_target_classes(
-                        subject_text,
+                        subject_text.as_str(content),
                         access_kind,
                         &rctx,
                     ),