Reduce repeated work when matching ignored errors

[zonuexe]

Summary

This reduces repeated work in IgnoredErrorHelperResult::process() in two ways:

• for an ignoreErrors entry containing paths, the identifier and message/regex are matched once instead of once per path;
• ignore entries with a different identifier are filtered out once per distinct error identifier before processing, while preserving their configured order.

The matching semantics and the behavior of entries using a singular path are unchanged.

Motivation

This was identified while profiling a large project with a warm result cache: phpstan/phpstan#15174 (reply in thread)

With no files requiring analysis, ignored-error processing was still a significant part of the warm run.

Benchmark

PHPStan 2.2.13, warm result cache, without the PHP baseline:

	Before	After matching message once per paths entry
Wall-clock median	5.26 s	4.38 s
Wall-clock mean	5.24 s	4.38 s
Samples	10	10

Matching messages once improved the median wall-clock time by 0.89 s (16.8%).

Filtering by identifier provided a further improvement in a bracketed comparison:

	Before identifier filtering	After identifier filtering
Wall-clock median	4.55 s	4.37 s
Samples	6	6

SPX profile

	Stock	Final patch
SPX process wall time	10.17 s	8.37 s
Total calls	22.82 M	16.28 M
IgnoredError::shouldIgnore() calls	2.52 M	0.38 M
Nette\Utils\Strings::match() calls	0.93 M	0.42 M
IgnoredErrorHelperResult::process()	3.23 s	1.13 s

Full-trace SPX adds substantial overhead, so the unprofiled wall-clock results above are the primary benchmark.

With the large PHP-format baseline enabled, identifier filtering improved the warm median from 5.02 s to 4.715 s in six samples per variant.

All stock and patched comparisons produced byte-identical output. They reported the same 1000+ errors without the PHP baseline and the same eight errors with it.

Tests

Regression coverage includes:

• matching a message and identifier against the second configured path;
• requiring the identifier to match when paths is used;
• retaining an unmatched identifier-specific entry for an exact file path.

Commands run:

vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserTest.php
php -d memory_limit=450M bin/phpstan analyse src/Analyser/Ignore/IgnoredErrorHelperResult.php tests/PHPStan/Analyser/AnalyserTest.php --no-progress

Results:

• PHPUnit: 62 tests, 204 assertions
• PHPStan: no errors

[zonuexe]

I have converted this PR to a draft while I work on another optimization in the same ignoreErrors processing loop.

After the current change, the warm SPX profile still shows approximately 0.89M calls to IgnoredError::shouldIgnore() and 0.42M calls to Nette\Utils\Strings::match(). I plan to reduce these calls by checking path applicability once per error file or trait context and only matching messages against applicable ignore rules.

I will add this optimization and regression tests for trait contexts, unmatched ignores, and count handling to this PR.

[zonuexe]

Update: I prototyped caching path applicability per file/trait context, but it did not improve the warm run (4.53 s before versus 4.57 s after), so I did not keep that change. Instead, I added identifier-based prefiltering that preserves configuration order and applies to both global and file-specific ignore entries. This reduced the warm median from 4.55 s to 4.37 s without the PHP baseline and from 5.02 s to 4.715 s with the large PHP baseline. The final SPX profile shows 0.38M IgnoredError::shouldIgnore() calls, down from 2.52M in stock. The branch and PR description have been updated with the implementation, tests, and benchmark results.

[phpstan-bot]

This pull request has been marked as ready for review.

[SanderMuller]

Tested this against head 6402eef75, because the warm-run ignore path is somewhere I have been measuring anyway. I am not the maintainer, so the calls are Ondřej's. The Windows failure @staabm caught is fixed and that whole column is green again, so nothing to re-raise there. Two things I can add: independent equivalence evidence, and a breakdown of where the win actually comes from.

Equivalence

21000 randomised cases over 7 seeds, byte-identical between base d6adcb4ac and this head, including after the ??= rewrite of the four cache guards. I drove IgnoredErrorHelperResult::process() directly with generated configurations covering every entry shape (bare string, message, rawMessage, identifier, identifiers, path, paths, count, reportUnmatched, and duplicate entries that merge) against generated errors with trait file paths, trait contexts and canBeIgnored: false, comparing the not-ignored errors, the ignored pairs and the other-ignore messages in order. One 4000-case dump held 1301 multi-path outcomes, 4308 unmatched reports, 198 count messages and 746 non-ignorable errors.

The harness is not blind. Breaking the prefilter (isset to array_key_exists) diverges by 292 lines; making the paths branch skip its message and identifier check diverges by 1765. Worth saying that my first version of it was blind: it called the array_key_exists mutant identical because it never generated two entries sharing (path, message, identifier), the shape IgnoredErrorHelper merges into an entry carrying identifier => null as a present key. That is the one place isset and array_key_exists differ, and isset is the one that agrees with shouldIgnore()'s ?? null.

Splitting the paths match is also sound structurally, not only empirically: shouldIgnore() is a conjunction of four independent predicates, so matching message and identifier once plus path per path is the same predicate as the combined call per path.

Reverting only the prefilter and keeping the paths restructure reproduces base output exactly, so the prefilter is behaviour-neutral on its own.

On a real corpus (4524 files, 3223 errors, a 2097-entry generated baseline) output is byte-identical base against head in all three entry shapes below, including the 2094 unmatched-ignore messages the paths shape produces.

I did wonder whether $identifierKey = $identifier ?? '' could collide with a genuine empty-string identifier, since the two want different filters. It cannot: Error::__construct() throws Invalid identifier: for '', which my fuzz found by trying to generate one. Non-issue, but the ??= keys rest on it.

Nothing here carries #[ShadowedByTurboExtension] and turbo-ext/ references neither class, so there is no C++ mirror to keep in step.

Where the win comes from

Warm runs, 0 files reanalysed, 3 clean interleaved rounds of 5 reps, CPU medians. The same 2097 entries rewritten three ways, so only the entry shape changes:

entry shape	base	head
as generated, all path-scoped (otherIgnoreErrors empty, 1.9 entries per file)	1.19 / 1.21 / 1.28 s	1.15 / 1.22 / 1.24 s
pathless (otherIgnoreErrors 2097, paths branch never runs)	1.66 / 1.63 / 1.64 s	1.37 / 1.34 / 1.39 s
paths with 3 paths each (both halves active)	3.24 / 3.65 / 3.43 s	1.59 / 1.69 / 1.67 s

So about -17% from the identifier prefilter on its own, about -52% with both halves, and flat on a plain generated baseline. Within-arm spread is 0.01 to 0.18 s, and I threw away one round where the machine was clearly contended rather than average it in.

That first row is the part I would want in the description. A --generate-baseline file is entirely path-scoped, so it puts nothing in otherIgnoreErrors and sees none of this; the win needs pathless or paths-scoped entries, which is presumably what your project's own ignoreErrors looks like. maxRSS is identical between arms in all three shapes, so the per-file and per-identifier caches cost nothing measurable at this scale. I did not build a project with both long per-file ignore lists and many distinct identifiers per file, so I cannot say what the $ignoreErrorsByFileAndIdentifier copies cost there.

One gap, not a blocker

The three added tests pass with src reverted to base, which is right for a refactor. They also pass under both mutations above, so the guard is coming from the pre-existing AnalyserTest (3 failures and 1 respectively), not from them. If you want a case that pins the prefilter specifically, it is the merge shape: two entries sharing (path, message, identifier) plus an error with a non-null identifier.

Gate on this head

Full suite 21320 tests / 96425 assertions green, self-analysis clean, phpcs clean on both files.

Remaining reds, none of them the change: Run with Turbo Extension (windows-latest, 8.3, nts, make tests) failed in setup-php with "Could not setup PHP 8.3" before anything ran, PHPStan (8.1, windows-latest) is red on base head too, and the Rector, Larastan, phpstan-laravel and dead-code-detector integration jobs plus extension-tests / phpstan-phpunit (7.4) and other-tests / rector-autoload are the base-wide set, identical on #6361 and #6373.

[staabm]

The harness is not blind. Breaking the prefilter (isset to array_key_exists) diverges by 292 lines; making the paths branch skip its message and identifier check diverges by 1765. Worth saying that my first version of it was blind: it called the array_key_exists mutant identical because it never generated two entries sharing (path, message, identifier), the shape IgnoredErrorHelper merges into an entry carrying identifier => null as a present key. That is the one place isset and array_key_exists differ, and isset is the one that agrees with shouldIgnore()'s ?? null.

@SanderMuller I am not sure how to read this. does it mean you can come up with a unit test which succceeded before this PR, but is now failling after the PR changes?

[SanderMuller]

No, and sorry for the ambiguity. I could not produce a test that passes on 2.2.x and fails on this PR, and that was what I spent the effort trying to do. The PR is behaviour-identical everywhere I looked.

Those two numbers are about hypothetical edits to the new code, not about the PR. I mutated the new code on purpose to check my harness could detect a break at all, because an equivalence check that cannot fail proves nothing. 292 lines is how far the harness diverges once I break the prefilter, so the 0 lines it reports for the PR itself means something.

The one worth knowing about is the isset in the new filter, because it is exactly the kind of line someone simplifies later. Your ?? suggestion was right and ??= keeps the semantics; the same reasoning one line further would not:

// two entries with the same path and message, no identifier
$entry = ['message' => '#^Fail\.$#', 'path' => $file];
new IgnoredErrorHelper($fileHelper, [$entry, $entry], true);

IgnoredErrorHelper merges those into one entry, and the merged shape carries identifier as a present key holding null:

{"message":"#^Fail\\.$#","rawMessage":null,"path":"...","identifier":null,"count":2,"reportUnmatched":true}
  isset(identifier)            = false
  array_key_exists(identifier) = true

So for an error in that file that does have an identifier:

2.2.x and this PR      ignored 1, not ignored 1 (the count shortfall message)
isset -> array_key_exists   ignored 0, not ignored 2 (the error itself is reported)

The codebase is not unguarded against that, to be clear: AnalyserTest::testIgnoreErrorByPathAndCount data sets #1, #2 and #4 fail on it, which is the same merge shape. What I meant to flag is only that the three tests added here pass under it, so if you wanted the new behaviour pinned by the new tests, that merged shape is the case to add. Not a blocker either way.

[staabm]

thank you both!