Improve conditional type resolving performance #2030

rajyan · 2022-11-27T03:34:09Z

resolves phpstan/phpstan#8397
resolves #2033
closes phpstan/phpstan#5805

I've improved the logic of conditionalExpressions to only resolve conditional types when needed, instead of resolving all conditionalExpressions every time filterBySpecifiedTypes is called.
I'm quite confident that this change can improve the performance :)
Now, all I have to do is to pass all the tests!

Update:

Instead of calling getType each time to check if conditional types's conditions are resolvable, I changed to only check if the conditions are specified and the specified type is same as the condition type.
This became possible, because previous refactoring made assignExpression and specifyExpressionType separated correctly, and $scope->getType can be used to get the specified types:+1:

… resolve types when needed

rajyan · 2022-11-27T06:36:15Z

653c00b
should fix the failing test, but would make a huge performance regression again.
I need a better way to resolve all conditionalTypes before merging scopes and createConditionalExpressions

ondrejmirtes · 2022-11-27T06:50:56Z

This looks promising 😊

Please run time make phpstan on 1.9.2, on 1.9.x and on your branch, so we can compare the numbers 😊 Thank you.

rajyan · 2022-11-27T07:10:32Z

I'm running hyperfine -i 'make phpstan' on every commit to check the performance
The current result is

# 1.9.2
  Time (mean ± σ):     18.332 s ±  0.975 s    [User: 114.153 s, System: 1.781 s]
  Range (min … max):   17.655 s … 20.206 s    10 runs

# 1.9.x
  Time (mean ± σ):     25.188 s ±  0.866 s    [User: 137.685 s, System: 1.855 s]
  Range (min … max):   24.466 s … 27.309 s    10 runs

# @42bddf02 (before the latest commit)
  Time (mean ± σ):     19.993 s ±  1.222 s    [User: 121.689 s, System: 1.802 s]
  Range (min … max):   19.240 s … 22.366 s    10 runs

ondrejmirtes · 2022-11-27T08:48:19Z

Looks great 😊

rajyan · 2022-11-28T00:12:35Z

This is something like resolving the conditional types as late as it can.
The problem I'm having now is, conditional types that could have been resolved is deleted before being resolved when merging scopes.
As we can see in

phpstan-src/src/Analyser/MutatingScope.php

Line 3904 in 42bddf0

    
           private function intersectConditionalExpressions(array $otherConditionalExpressions): array

when merging, we can only "intersect" conditional types.

simple example

if ($a) {
  // conditional type $c='foo' => $b = 'foo'
} else {
  // conditional type $c='bar' => $b = 'bar'
}

// Whats the conditional type here? Currently they are deleted while intersecting
// Maybe we can merge them like $a=truthty&&$c='foo' => $b='foo', $a=falsy&&$c='bar' => $b='bar'

This reverts commit 653c00b.

rajyan · 2022-11-28T00:57:31Z

Three failing tests are related to createConditionalExpressions
One is related to this commented out lines
https://github.com/phpstan/phpstan-src/pull/2030/files#diff-dc817f2bab8672057a95375a542e68599164f6ab2380e4cccd1b50583e295d85R3580-R3589

ondrejmirtes · 2022-11-28T09:49:39Z

Unfortunately you're on your own, I hope you figure it out, I'm following it closely :) It'd take me a few days to load everything into my brain and it's already in yours so you'll be faster in finishing it I'm sure :)

rajyan · 2022-11-29T00:34:25Z

I debugged further
https://gist.github.com/rajyan/e8b66dc43c4df4fef0d6fcc139ca0597

bug-987 shows whats happening

phpstan-src/tests/PHPStan/Analyser/data/bug-987.php

Lines 38 to 51 in 579b075

    
           function hello(self $class, bool $boolOption): array 
        
           { 
        
           	$myArray = $class->arrOrNull(); 
        
           	if ($boolOption && $myArray === null) { 
        
           		throw new \Exception(''); 
        
           	} 
        
           	if (!$boolOption) { 
        
           		$myArray = $class->otherGetArray(); 
        
           	} 
        
           	assertType('array<string>', $myArray); 
        
           	return $myArray; 
        
           }

	/**
	 * @return string[]
	 */
	function hello(self $class, bool $boolOption): array
	{
		$myArray = $class->arrOrNull();
		if ($boolOption && $myArray === null) {
			throw new \Exception('');
		}
		// !$boolOption or $boolOption=true => $myArray=array<string>
		if (!$boolOption) {
			$myArray = $class->otherGetArray();
			// $myArray=array<string> in inner scope
		}
                // inner scope filterByTruthyValue(!$boolOption) and outer scope  filterByFalsyValue(!$boolOption) is merged
		// In outer scope $boolOption=true, so it can resolve $boolOption=true => $myArray=array<string>
                // Although, with changes in fix-conditional-performance this conditional is not resolved and still $myArray=array<string>|null

		assertType('array<string>', $myArray);

		return $myArray;
	}

rajyan · 2022-11-29T00:57:20Z

I think adding this kind of conditional type
phpstan/phpstan#8360 (comment)
can solve this problem.
Maybe something can be improved in

phpstan-src/src/Analyser/NodeScopeResolver.php

Lines 3420 to 3427 in f8f09cd

    
           $truthyType = TypeCombinator::remove($type, StaticTypeFactory::falsey()); 
        
           $falseyType = TypeCombinator::intersect($type, StaticTypeFactory::falsey()); 
        
           $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); 
        
           $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $truthySpecifiedTypes, $truthyType); 
        
           $conditionalExpressions = $this->processSureTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType); 
        
           $conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($scope, $var->name, $conditionalExpressions, $falseySpecifiedTypes, $falseyType);

but not sure yet.

rajyan · 2022-11-30T01:23:00Z

src/Analyser/MutatingScope.php

+		foreach ($scope->conditionalExpressions as $conditionalExprString => $conditionalExpressions) {
+			foreach (array_reverse($conditionalExpressions) as $conditionalExpression) {
+				$newConditionExpressionTypeHolders = [];
+				foreach ($conditionalExpression->getConditionExpressionTypeHolders() as $holderExprString => $conditionalTypeHolder) {
+					if ($holderExprString === $exprString && $expressionTypes[$holderExprString]->equals($conditionalTypeHolder)) {
+						continue;
+					}
+					$newConditionExpressionTypeHolders[$holderExprString] = $conditionalTypeHolder;
+				}
+				if ($newConditionExpressionTypeHolders === []) {
+					if ($conditionalExpression->getTypeHolder()->getCertainty()->no()) {
+						unset($expressionTypes[$conditionalExprString]);
+					} else {
+						$expressionTypes[$conditionalExprString] = $conditionalExpression->getTypeHolder();
+					}
+					continue 2;
+				}
+			}
+		}


I'm not satisfied enough with this implementation, but this works at least.
I think after conditional types merging logic has improved, we can delete matching ConditionExpressionTypeHolders

rajyan · 2022-11-30T01:37:01Z

src/Analyser/MutatingScope.php

+		// tmp: causes an infinite loop by `getType` in `findPropertyReflectionFromNode`
+		//      if ($expr instanceof PropertyFetch) {
+		//          $propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($expr, $this);
+		//          if ($propertyReflection !== null) {
+		//              $nativePropertyReflection = $propertyReflection->getNativeReflection();
+		//              if ($nativePropertyReflection !== null && $nativePropertyReflection->isReadOnly()) {
+		//                  return false;
+		//              }
+		//          }
+		//      }


Now the only thing to solve is this

Waiting impatiently for the fix :)

only have to invalidate the current conditional expression

rajyan · 2022-12-01T01:04:26Z

Hmm
this rector integration test error

 ------ ----------------------------------------------------------------------- 
  Line   rules/Naming/Naming/ExpectedNameResolver.php                           
 ------ ----------------------------------------------------------------------- 
  187    Parameter 1 should use "PHPStan\Type\ArrayType" type as the only type  
         passed to this method                                                  
 ------ -----------------------------------------------------------------------

doesn't look correct.

rajyan · 2022-12-02T00:17:03Z

I'm very satisfied with this implementation. Gonna update the pull request description for explanation!

…pecified and types are same

phpstan-bot · 2022-12-02T00:33:32Z

This pull request has been marked as ready for review.

rajyan · 2022-12-02T00:40:58Z

╭─ ~/Contribution/phpstan-src  fix-conditional-performance *5 ·································································································· INT ✘  1m 5s  09:27:59 
╰─ hyperfine -i 'make phpstan'
Benchmark 1: make phpstan
  Time (mean ± σ):     20.455 s ±  0.942 s    [User: 118.691 s, System: 1.995 s]
  Range (min … max):   19.193 s … 21.805 s    10 runs
 
╭─ ~/Contribution/phpstan-src  fix-conditional-performance *5 ····································································································· ✔  3m 25s  09:31:47 
╰─ git switch --detach 1.9.2
HEAD is now at 582a9cb8b Fix unset on nested array
╭─ ~/Contribution/phpstan-src  #1.9.2 *5 ··································································································································· ✔  09:32:07 
╰─ hyperfine -i 'make phpstan'
Benchmark 1: make phpstan
  Time (mean ± σ):     18.278 s ±  0.598 s    [User: 114.610 s, System: 1.880 s]
  Range (min … max):   17.741 s … 19.564 s    10 runs

 
╭─ ~/Contribution/phpstan-src  #1.9.2 *5 ··························································································································· ✔  3m 3s  09:35:11 
╰─ git switch --detach 1.9.x  
Previous HEAD position was 582a9cb8b Fix unset on nested array
HEAD is now at 70a87cceb ImagickPixel::getColor() normalized param accepts int instead of bool
Your branch is up to date with 'origin/1.9.x'.
╭─ ~/Contribution/phpstan-src  @70a87cce *5 ································································································································ ✔  09:35:19 
╰─ hyperfine -i 'make phpstan'
Benchmark 1: make phpstan
  Time (mean ± σ):     25.497 s ±  0.776 s    [User: 138.297 s, System: 1.844 s]
  Range (min … max):   24.355 s … 26.668 s    10 runs

ondrejmirtes

Awesome, looks great 😊

Please add a regression test for #5805, check the issue bot.
Did you check all the failures? Are they improvements?

thank you!

rajyan · 2022-12-02T07:39:48Z

Please add a regression test for #5805, check the issue bot.

Oh, thanks! I wasn't aware with the issue bot result change.
I'll add it as soon as I get back home:)

Did you check all the failures? Are they improvements?

Sorry I forgot to comment about it.
The only changed test result is
https://github.com/phpstan/phpstan-src/actions/runs/3597748538/jobs/6059882549
and I believe this is a improvement

https://github.com/PrestaShop/PrestaShop/blob/38d8ad3ef7a2faa6b6db73b0a19768d2be3dc85c/controllers/admin/AdminImportController.php#L3094-L3101
here $customer_exist is only set to true once, so isset($current_id_customer), isset($current_id_shop_group), isset($current_id_shop) is always true if $customer_exist = true

https://github.com/PrestaShop/PrestaShop/blob/38d8ad3ef7a2faa6b6db73b0a19768d2be3dc85c/controllers/admin/AdminProductsController.php#L689
the isset is redundant because conditional type with expression Configuration::get('PS_ADVANCED_STOCK_MANAGEMENT') is now working and we know that $stock_manager is always assigned in line 687.

herndlm · 2022-12-02T07:53:11Z

Very nice indeed 🎉

rajyan · 2022-12-02T07:58:13Z

Confirm that added regression test fails with 1.9.x

ondrejmirtes · 2022-12-02T08:01:25Z

Thank you!

I'll try to run it with Blackfire to compare 1.9.2 and the new 1.9.x to see what can be done, after the weekend.

staabm · 2022-12-02T08:52:33Z

great job @rajyan 👍

rajyan added 3 commits November 27, 2022 12:25

extract conditionalExpression resolving as a private method, and only…

ba4d71b

… resolve types when needed

intersect resolved conditional type with expression type

42bddf0

tmp have to resolve all types before merging

653c00b

Revert "tmp have to resolve all types before merging"

1b5c3d4

This reverts commit 653c00b.

janedbal mentioned this pull request Nov 28, 2022

Example test of huge slowdown in 1.9.x #2033

Closed

Example test of slowdown in 1.9.x

3a73c5b

rajyan added 3 commits November 30, 2022 10:00

fix

6df6aa8

filter $conditionExpressionTypeHolders when specified

2c404b7

tmp keep conditional type

a72f632

rajyan force-pushed the fix-conditional-performance branch from c6d605b to a72f632 Compare November 30, 2022 01:01

fix resolving getCertainty()->no()

db446e8

rajyan commented Nov 30, 2022

View reviewed changes

cs fix early return

9e8d82c

rajyan commented Nov 30, 2022

View reviewed changes

fix infinite loop

7d2125d

only have to invalidate the current conditional expression

rajyan added 2 commits December 2, 2022 08:07

clean up resolving conditional types when specifying

c2cbcf9

clean up

68afcd8

rajyan force-pushed the fix-conditional-performance branch from e07daae to 68afcd8 Compare December 1, 2022 23:18

add resolved conditional types to specified expressions

4fe0408

rajyan changed the title ~~extract conditionalExpression resolving as a private method, and only…~~ Improve conditional type resolving performance Dec 2, 2022

refactor: conditional type is resolvable only if all conditions are s…

0c86a04

…pecified and types are same

rajyan marked this pull request as ready for review December 2, 2022 00:33

ondrejmirtes requested changes Dec 2, 2022

View reviewed changes

add regression test #5803 from issue bot

687e490

ondrejmirtes merged commit 6c0f6eb into phpstan:1.9.x Dec 2, 2022

herndlm mentioned this pull request Dec 2, 2022

Improve expression resolving of superglobals #2012

Merged

rajyan deleted the fix-conditional-performance branch December 2, 2022 10:27

Improve conditional type resolving performance #2030

Improve conditional type resolving performance #2030

Uh oh!

Conversation

rajyan commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Nov 27, 2022

Uh oh!

rajyan commented Nov 27, 2022

Uh oh!

ondrejmirtes commented Nov 27, 2022

Uh oh!

rajyan commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan commented Nov 28, 2022

Uh oh!

ondrejmirtes commented Nov 28, 2022

Uh oh!

rajyan commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan commented Nov 29, 2022

Uh oh!

rajyan Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

janedbal Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan commented Dec 1, 2022

Uh oh!

rajyan commented Dec 2, 2022

Uh oh!

phpstan-bot commented Dec 2, 2022

Uh oh!

rajyan commented Dec 2, 2022

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

rajyan commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

herndlm commented Dec 2, 2022

Uh oh!

rajyan commented Dec 2, 2022

Uh oh!

ondrejmirtes commented Dec 2, 2022

Uh oh!

staabm commented Dec 2, 2022

Uh oh!

Uh oh!

rajyan commented Nov 27, 2022 •

edited

Loading

rajyan commented Nov 27, 2022 •

edited

Loading

rajyan commented Nov 28, 2022 •

edited

Loading

rajyan commented Nov 29, 2022 •

edited

Loading

rajyan commented Dec 2, 2022 •

edited

Loading