Implement OversizedArrayVisitor to improve huge constant array performance #2116

staabm · 2022-12-15T18:02:54Z

implements the idea described in phpstan/phpstan#8215 (comment)

closes phpstan/phpstan#8215

with this fix, the snippet is analyzable in 2 seconds, while it takes several minutes before.

staabm · 2022-12-15T18:07:59Z

it needs a few fixes for existing tests, but what do you think about the approach in general?

ondrejmirtes · 2022-12-16T08:33:47Z

Can you explain your reasoning behind this code? I don't entirely understand it.

staabm · 2022-12-16T08:41:26Z

the idea is, that phpstan is slow when analyzing huge arrays, like gist because it tries to get the most precise result.

we already have an OversizedArrayType, but it works in a way that it needs to build up the constant array using ConstantTypes and then collapsing it into a OversizedArrayType, after realizing its too big. in huge cases even the steps to build up the Constant types and collapse them into OversizedArrayType already takes minutes.

this PR tries to add a very lightweight, but imprecise type inference when very big constant arrays are analyzed.
we determine a rough shape based on only the simplest types, to make sure we don't later run into the very slow Constant types -> OversizedArrayType case.

see the blackfire profile of said gist, where we are super slow because of the sheer number of constant-integer types invovled

ondrejmirtes · 2022-12-16T08:49:57Z

Does this also make https://github.com/phpstan/phpstan-src/blob/aa7b1eabd197ef6b7fb2c8eebbb7876aebc0ba6a/tests/PHPStan/Analyser/data/bug-8146.php fast?
Does this make https://github.com/phpstan/phpstan-src/blob/98c66f30dea8bbb075e327cb5c415427000446e8/tests/PHPStan/Analyser/data/bug-8146a.php fast?
Does this get rid of false positives when an array type is simplified but the array is returned from a method that typehints a precise array shape?

I need at least 1) and 3) to be true. I understand that 2) might be a different problem.

staabm · 2022-12-16T09:39:04Z

Does this also make aa7b1ea/tests/PHPStan/Analyser/data/bug-8146.php fast?

runs locally in 13 seconds. on the latest 1.9.x-dev it runs in 1min 18 secs.

2. Does this make 98c66f3/tests/PHPStan/Analyser/data/bug-8146a.php fast?

I don't think this counts as a constant array in a sense this Visitor could improve. analyzing this source requires a already built up scope, or at least knowledge about the returned types of methods.

3. Does this get rid of false positives when an array type is simplified but the array is returned from a method that typehints a precise array shape?

I will work on this one now

staabm · 2022-12-16T11:28:18Z

src/Analyser/TypeSpecifier.php

@@ -1080,31 +1076,7 @@ private function specifyTypesForConstantStringBinaryExpression(
 			&& strtolower($exprNode->name->toString()) === 'gettype'
 			&& isset($exprNode->getArgs()[0])
 		) {
-			$type = null;
-			if ($constantType->getValue() === 'string') {


extracted into new helper for re-use in new visitor

phpstan-bot · 2022-12-16T11:33:46Z

This pull request has been marked as ready for review.

src/Rules/FunctionReturnTypeCheck.php

phpstan-bot · 2022-12-18T09:47:09Z

This pull request has been marked as ready for review.

staabm · 2022-12-18T09:54:54Z

Backward Compatibility

fails because I have removed a @deprecated annotation - I guess.
I think its a false positive

staabm · 2022-12-18T09:58:08Z

phpstan/phpstan#8215 sees a ~25x improvement

➜  phpstan-src git:(oversized-array) ✗ php bin/phpstan analyze tests/PHPStan/Analyser/data/bug-8146b.php --debug -vvv
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
Result cache not used because of debug mode.
    0 [▓▓▓░░░░░░░░░░░░░░░░░░░░░░░░░] < 1 sec 24.0 MiB

Result cache was not saved because only files were passed as analysed paths.
 [OK] No errors

Used memory: 74 MB
➜  phpstan-src git:(oversized-array) ✗ php bin/phpstan analyze test.php --debug -vvv
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
Result cache not used because of debug mode.
/Users/staabm/workspace/phpstan-src/test.php
--- consumed 91.98 MB, total 142 MB, took 23.03 s
Result cache was not saved because only files were passed as analysed paths.
 [OK] No errors

Used memory: 142 MB

phpstan/phpstan#8146 (comment) sees a 2x improvement

➜  phpstan-src git:(oversized-array) ✗ php bin/phpstan analyze test.php --debug -vvv
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
Result cache not used because of debug mode.
/Users/staabm/workspace/phpstan-src/test.php
--- consumed 109.98 MB, total 160 MB, took 21.31 s
Result cache was not saved because only files were passed as analysed paths.

➜  phpstan-src git:(1.9.x) ✗ php bin/phpstan analyze test.php --debug -vvv
Note: Using configuration file /Users/staabm/workspace/phpstan-src/phpstan.neon.dist.
Result cache not used because of debug mode.
/Users/staabm/workspace/phpstan-src/test.php
--- consumed 136 MB, total 160 MB, took 42.41 s

ondrejmirtes · 2022-12-18T15:12:45Z

I don't get what's exactly happening in the visitor, it's a lot of code.

And I don't think it has to be a visitor, to me it looks like it's essentially caching for something that could be in InitializerExprTypeResolver where Array_ type is resolved.

staabm · 2022-12-18T15:50:52Z

ohh thats a good point. let me see whether I can get rid of the Visitor.

staabm · 2022-12-18T16:20:29Z

great suggestion, it got even simpler now.

phpstan-bot · 2022-12-18T16:23:26Z

This pull request has been marked as ready for review.

src/Reflection/InitializerExprTypeResolver.php

staabm · 2022-12-19T10:16:23Z

src/Reflection/InitializerExprTypeResolver.php

+				$isList = false;
+
+				$itemKeyType = $getTypeCallback($key);
+				if (!$itemKeyType instanceof ConstantScalarType || $key instanceof ClassConstFetch) {


We skip const class fetch, as I was not able to make ArrayType accept a OversizedArrayType with reduced precision when using Constant enumerations like self::SOME_*.
See failing test when removing this check

staabm · 2022-12-19T10:18:01Z

src/Reflection/InitializerExprTypeResolver.php

+			}
+
+			$itemValueType = $getTypeCallback($value);
+			if (!$itemValueType instanceof ConstantScalarType || $value instanceof ClassConstFetch) {


I think we could also add support for arrays where const arrays call methods/functions as long as the return type is defined.

example:

… public function getData(): array { return [ 'Bács-Kiskun' => [ 'Ágasegyháza' => [ 'constituencies' => ['Bács-Kiskun 4.'], 'coordinates' => ['lat' => 46.8386043, 'lng' => 19.4502899], ], $this->returnsString() => [ 'constituencies' => ['Bács-Kiskun 3.'], 'coordinates' => ['lat' => 46.6898175, 'lng' => 19.205086], ], 'Apostag' => [ 'constituencies' => ['Bács-Kiskun 3.'], 'coordinates' => ['lat' => 46.8812652, 'lng' => $this->returnsFloat()], ],

I did not yet work on it, as I think we are still discussing whether the change itself is acceptable.

ondrejmirtes · 2022-12-19T11:32:01Z

I just pushed a simplification of the buildDegradedConstantArray method as I imagine it should work.

A few problems with it though - some tests fail. I think that we'll have to modify not only accepts, but isSuperTypeOf as well.

Also - it's not always correct. When $key === null needs to be handled, and also $isList isn't always truthful.

ondrejmirtes · 2022-12-20T08:37:02Z

Hi, I'm taking over this PR and I'll try to finish it. Thanks.

staabm · 2022-12-20T09:20:10Z

I tried several things which didn't work. Thanks for taking it

staabm · 2022-12-20T11:06:43Z

tests/PHPStan/Rules/Methods/ReturnTypeRuleTest.php

@@ -809,4 +809,15 @@ public function testBug8223(): void
 		]);
 	}

+	public function testBug8146b(): void


nit: I misstyped the testname

Suggested change

public function testBug8146b(): void

public function testBug8146c(): void

…mance

phpstan-bot · 2022-12-20T21:21:48Z

This pull request has been marked as ready for review.

ondrejmirtes · 2022-12-20T21:27:42Z

Thank you.

staabm · 2022-12-20T21:31:48Z

Thank you 🙏

ddebin · 2023-01-03T10:43:51Z

Do you think we can disable this new behavior? Now, my huge const is oversimplified and the array shape is not guessed as precisely as before.

ondrejmirtes · 2023-01-03T10:44:50Z

@ddebin Please open a new issue and make sure to include a code sample.

ddebin · 2023-01-03T11:11:45Z

@ondrejmirtes phpstan/phpstan#8636

staabm force-pushed the oversized-array branch 3 times, most recently from ef9f751 to c3955a3 Compare December 16, 2022 11:26

staabm commented Dec 16, 2022

View reviewed changes

staabm force-pushed the oversized-array branch 2 times, most recently from 04ea982 to a732639 Compare December 16, 2022 11:33

staabm marked this pull request as ready for review December 16, 2022 11:33

staabm force-pushed the oversized-array branch 2 times, most recently from 19f2442 to b33c3cc Compare December 16, 2022 11:41

ondrejmirtes reviewed Dec 16, 2022

View reviewed changes

src/Rules/FunctionReturnTypeCheck.php Outdated Show resolved Hide resolved

staabm force-pushed the oversized-array branch from b33c3cc to 500ae8a Compare December 16, 2022 14:45

staabm marked this pull request as draft December 16, 2022 15:29

staabm force-pushed the oversized-array branch from 51baa57 to 36e9381 Compare December 18, 2022 09:37

staabm marked this pull request as ready for review December 18, 2022 09:46

staabm marked this pull request as draft December 18, 2022 15:50

staabm marked this pull request as ready for review December 18, 2022 16:23

ondrejmirtes requested changes Dec 18, 2022

View reviewed changes

src/Reflection/InitializerExprTypeResolver.php Show resolved Hide resolved

staabm force-pushed the oversized-array branch from e972230 to 690a128 Compare December 18, 2022 21:48

samsonasik mentioned this pull request Dec 19, 2022

Rector hangs on a particular file rectorphp/rector#7649

Closed

staabm commented Dec 19, 2022

View reviewed changes

staabm force-pushed the oversized-array branch from 761c269 to 905a21f Compare December 19, 2022 12:07

staabm marked this pull request as draft December 19, 2022 12:07

ondrejmirtes force-pushed the oversized-array branch 2 times, most recently from 9ebf4a4 to 1573a79 Compare December 20, 2022 11:00

staabm commented Dec 20, 2022

View reviewed changes

ondrejmirtes force-pushed the oversized-array branch 2 times, most recently from 248dd35 to 7d40377 Compare December 20, 2022 14:34

Implement OversizedArrayBuilder to improve huge constant array perfor…

f1e0ed2

…mance

ondrejmirtes force-pushed the oversized-array branch from 7d40377 to f1e0ed2 Compare December 20, 2022 21:21

ondrejmirtes marked this pull request as ready for review December 20, 2022 21:21

ondrejmirtes approved these changes Dec 20, 2022

View reviewed changes

ondrejmirtes merged commit ae37fb9 into phpstan:1.9.x Dec 20, 2022

ondrejmirtes mentioned this pull request Dec 20, 2022

Performance problem - big arrays phpstan/phpstan#8504

Closed

staabm deleted the oversized-array branch December 21, 2022 11:03

ddebin mentioned this pull request Jan 3, 2023

Oversimplified huge const array phpstan/phpstan#8636

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OversizedArrayVisitor to improve huge constant array performance #2116

Implement OversizedArrayVisitor to improve huge constant array performance #2116

staabm commented Dec 15, 2022 •

edited

staabm commented Dec 15, 2022

ondrejmirtes commented Dec 16, 2022

staabm commented Dec 16, 2022 •

edited

ondrejmirtes commented Dec 16, 2022

staabm commented Dec 16, 2022

staabm Dec 16, 2022

phpstan-bot commented Dec 16, 2022

phpstan-bot commented Dec 18, 2022

staabm commented Dec 18, 2022

staabm commented Dec 18, 2022 •

edited

ondrejmirtes commented Dec 18, 2022

staabm commented Dec 18, 2022

staabm commented Dec 18, 2022 •

edited

phpstan-bot commented Dec 18, 2022

staabm Dec 19, 2022

staabm Dec 19, 2022 •

edited

ondrejmirtes commented Dec 19, 2022

ondrejmirtes commented Dec 20, 2022

staabm commented Dec 20, 2022 •

edited

staabm Dec 20, 2022

phpstan-bot commented Dec 20, 2022

ondrejmirtes commented Dec 20, 2022

staabm commented Dec 20, 2022

ddebin commented Jan 3, 2023

ondrejmirtes commented Jan 3, 2023

ddebin commented Jan 3, 2023

	public function testBug8146b(): void
	public function testBug8146c(): void

Implement OversizedArrayVisitor to improve huge constant array performance #2116

Implement OversizedArrayVisitor to improve huge constant array performance #2116

Conversation

staabm commented Dec 15, 2022 • edited

staabm commented Dec 15, 2022

ondrejmirtes commented Dec 16, 2022

staabm commented Dec 16, 2022 • edited

ondrejmirtes commented Dec 16, 2022

staabm commented Dec 16, 2022

staabm Dec 16, 2022

Choose a reason for hiding this comment

phpstan-bot commented Dec 16, 2022

phpstan-bot commented Dec 18, 2022

staabm commented Dec 18, 2022

staabm commented Dec 18, 2022 • edited

ondrejmirtes commented Dec 18, 2022

staabm commented Dec 18, 2022

staabm commented Dec 18, 2022 • edited

phpstan-bot commented Dec 18, 2022

staabm Dec 19, 2022

Choose a reason for hiding this comment

staabm Dec 19, 2022 • edited

Choose a reason for hiding this comment

ondrejmirtes commented Dec 19, 2022

ondrejmirtes commented Dec 20, 2022

staabm commented Dec 20, 2022 • edited

staabm Dec 20, 2022

Choose a reason for hiding this comment

phpstan-bot commented Dec 20, 2022

ondrejmirtes commented Dec 20, 2022

staabm commented Dec 20, 2022

ddebin commented Jan 3, 2023

ondrejmirtes commented Jan 3, 2023

ddebin commented Jan 3, 2023

staabm commented Dec 15, 2022 •

edited

staabm commented Dec 16, 2022 •

edited

staabm commented Dec 18, 2022 •

edited

staabm commented Dec 18, 2022 •

edited

staabm Dec 19, 2022 •

edited

staabm commented Dec 20, 2022 •

edited