Skip to content

NodeScopeResolver: 10x Faster constant array processing #3769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 4, 2025

vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserIntegrationTest.php --filter testBug11913 triggers the by-far slowest unit test in the phpstan-src codebase

before this PR it takes 10-11 seconds
after this PR it finishes in 0.3-0.4 seconds
(run on macbook pro m4 with PHP 8.3.15)


the code is dominated by calculating intersections in

$offsetValueType = TypeCombinator::intersect($offsetValueType, TypeCombinator::union(...$types));

my thesis is, that when we know a array is constant, it will always intersect with new ArrayType(new MixedType(), new MixedType()) and therefore don't need to take the slow path

@staabm staabm marked this pull request as ready for review January 4, 2025 14:38
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is this relevant on 1.12.x as well?
  2. What about general array?
  3. What I'd like to attempt is doing TypeCombinator::union(...$types)->isSuperTypeOf($offsetValueType) and if it's not a yes() only then run the intersect(). Basically we want to run it only if it's not an array, an array access...

@staabm
Copy link
Contributor Author

staabm commented Jan 4, 2025

  • Is this relevant on 1.12.x as well?

the reproducer is equal slow on 1.12.x, but it does not get faster with this patch (bottleneck on 1.12.x is elsewhere):

1.12.x profile of bug-11913.php

grafik
  • What about general array?

I think general arrays are less of interesst perf wise because they usually don't tend to get equal huge as constant arrays.

using && !$offsetValueType->isArray()->yes()
instead of && !$offsetValueType->isConstantArray()->yes()

is equal fast but yields the following regressions:

1) PHPStan\Analyser\Bug9307Test::testFileAsserts with data set "/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/data/bug-9307.php:34" ('type', '/Users/staabm/workspace/phpst...07.php', 'array', 'array<*ERROR*>', 34)
Expected type array, got type array<*ERROR*> in /Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/data/bug-9307.php on line 34.
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array'
+'array<*ERROR*>'

/Users/staabm/workspace/phpstan-src/src/Testing/TypeInferenceTestCase.php:124
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/Bug9307Test.php:25

2) PHPStan\Rules\Functions\CallToFunctionParametersRuleTest::testBug2911
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'23: Parameter #1 $array of function Bug2911\bar expects array{bar: string}, non-empty-array given.
+'23: Parameter #1 $array of function Bug2911\bar expects array{bar: string}, non-empty-array<mixed> given.
 '

/Users/staabm/workspace/phpstan-src/src/Testing/RuleTestCase.php:161
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Rules/Functions/CallToFunctionParametersRuleTest.php:1280

3) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "tests/PHPStan/Analyser/nsrt/bug-11518-types.php" ('/Users/staabm/workspace/phpst...es.php')
Failed assertions in /Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/nsrt/bug-11518-types.php:

Line 15:
Expected: non-empty-array&hasOffsetValue('thing', 'bla')
Actual:   non-empty-array<mixed>&hasOffsetValue('thing', 'bla')

/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/NodeScopeResolverTest.php:265

4) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "tests/PHPStan/Analyser/nsrt/bug-2911.php" ('/Users/staabm/workspace/phpst...11.php')
Failed assertions in /Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/nsrt/bug-2911.php:

Line 137:
Expected: non-empty-array&hasOffsetValue('bar', 'string')
Actual:   non-empty-array<mixed>&hasOffsetValue('bar', 'string')

  • What I'd like to attempt is doing TypeCombinator::union(...$types)->isSuperTypeOf($offsetValueType) and if it's not a yes() only then run the intersect(). Basically we want to run it only if it's not an array, an array access...

tried it, but this did not work. it prevents the slow TypeCombinator::intersect but the newly added TypeCombinator::union(...$types)->isSuperTypeOf($offsetValueType) is equal slow to the intersect call which is prevented

if (!$offsetValueType instanceof MixedType) {
if (
!$offsetValueType instanceof MixedType
&& !$offsetValueType->isConstantArray()->yes()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to comment this is for performance only

@ondrejmirtes ondrejmirtes merged commit 7c281ba into phpstan:2.1.x Jan 5, 2025
426 checks passed
@ondrejmirtes
Copy link
Member

Alright, let's try this! Thank you.

@staabm staabm deleted the fast branch January 5, 2025 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants