Perf: cache UseStatementSniff getUseStatements across fix iterations#66
Merged
Merged
Conversation
UseStatementSniff has its own getUseStatements() implementation that duplicates UseStatementsTrait::getUseStatements() (cached in #64). It already has an instance-level cache via existingStatements, which covers repeated calls within a single phpcs pass; the new static cache adds coverage across phpcbf fix iterations, where populateTokenListeners() creates fresh sniff instances per pass and resets the instance cache. Cache invalidation follows the same fingerprint-based scheme as #64: token count alone is not strong enough, since an alias rename keeps it constant. Cached entries record a content fingerprint of each use statement range and re-verify them against the live tokens before being trusted. The cache also refuses to serve an empty result so it cannot return stale state for a file where a fix added a first use statement while another simultaneous fix happened to keep the file's overall token count unchanged. Measured on the same CakePHP 5 app from #62 / #63 / #64 / #65 (parallel=1, --report=performance): PhpCollective.Namespaces.UseStatement 3.36s -> 2.08s Existing test suite (100 tests / 122 assertions) passes unchanged. The FQCN cache changes from the earlier draft of this PR were dropped in favour of the cache that landed via #65.
dereuromark
force-pushed
the
perf-cache-usestatement-and-fqcn
branch
from
2178955 to
9f2a6d7
Compare
dereuromark
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a per-file cache to
UseStatementSniff::getUseStatements(), the same shape as #64 and #65.UseStatementSniffhas its owngetUseStatements()implementation that duplicatesUseStatementsTrait::getUseStatements()(the one cached in #64). The existing instance-level cache via$existingStatementscovers repeated calls within a single phpcs pass; the new static cache adds coverage across phpcbf fix iterations, wherepopulateTokenListeners()creates fresh sniff instances per pass and resets the instance cache.Cache invalidation
Same fingerprint-based scheme introduced in #64. Token count alone is not strong enough — an alias rename keeps it constant. Cached entries record a content fingerprint of each use statement range and re-verify them against the live tokens before being trusted.
Defensive belt-and-braces: refuse to serve a cached empty result. Files with no
usestatements don't benefit from caching anyway, and skipping the cache for empty results means we won't serve stale state in the corner case where a fix adds a first use statement while another simultaneous fix happens to keep the overall token count unchanged.Why not the trait
UseStatementSniff::getUseStatements()is almost a duplicate ofUseStatementsTrait::getUseStatements()(the one cached in #64), but with a slightly different result shape (no'statement'field). Refactoring this sniff onto the trait is doable but out of scope for a perf-only PR — same cache shape, kept local.Benchmarks
Measured on the same 1095-file CakePHP 5 app from #62 / #63 / #64 / #65. Per-sniff CPU times from
phpcs --parallel=1 --report=performance:PhpCollective.Namespaces.UseStatementNote
An earlier draft of this PR also added a cache to
FullyQualifiedClassNameInDocBlockSniff::parseUseStatements()/::getNamespace(). Those changes were dropped after #65 landed with a different (monotonic-stack-pointer) caching scheme covering the same hot path.Verification
composer cs-checkandcomposer stanon this repo: clean.codex review --uncommittedon the original (broader) draft flagged P2 cache-invalidation concerns; the empty-result skip in this final version was added in response.