Skip to content
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

Added regression test #3062

Merged
merged 2 commits into from May 11, 2024
Merged

Added regression test #3062

merged 2 commits into from May 11, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 10, 2024

closes phpstan/phpstan#10979

reverting the fix for #2985 does not regress the test of said PR.
I need to dive deeper why the test no longer reproduces the perf problem we had at the time #2985 was created

I had a typo locally .. investigating..

@@ -201,7 +200,8 @@ public static function union(Type ...$types): Type
if ($types[$i] instanceof StringType && !$types[$i] instanceof ClassStringType) {
$hasGenericScalarTypes[ConstantStringType::class] = true;
}
if ($types[$i] instanceof EnumCaseObjectType) {
$enumCases = $types[$i]->getEnumCases();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverts #2985

@staabm staabm marked this pull request as ready for review May 10, 2024 12:56
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor Author

staabm commented May 10, 2024

after this PR we now have both cases fast:

php bin/phpstan analyze --debug bug-10772.php  1.30s user 0.14s system 98% cpu 1.451 total


php bin/phpstan analyze --debug LibraryMasterEnum.php  3.16s user 0.14s system 99% cpu 3.328 total

@staabm
Copy link
Contributor Author

staabm commented May 10, 2024

will be AFK for a few hours

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.

Yeah this looks good 👍

@@ -94,6 +94,9 @@ class ObjectType implements TypeWithClassName, SubtractableType

private ?string $cachedDescription = null;

/** @var array<class-string, list<EnumCaseObjectType>> */
private static array $enumCasesCache = [];
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be reset in resetCaches()

foreach ($this->subtractedType->getEnumCases() as $enumCase) {
$subtracted[$enumCase->getEnumCaseName()] = true;
$className = $classReflection->getName();
if (array_key_exists($className, self::$enumCasesCache)) {
Copy link
Member

Choose a reason for hiding this comment

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

The cache key needs to be $this->describeCache().

@staabm staabm force-pushed the fix2 branch 3 times, most recently from 69a9068 to 168d145 Compare May 10, 2024 18:15
@staabm
Copy link
Contributor Author

staabm commented May 10, 2024

this one is good to go. I will have a look at the other remaining open PRs after this one is merged, whether there is still meaningful impact

php bin/phpstan analyze --debug LibraryMasterEnum.php  
  2.02s user 0.13s system 99% cpu 2.160 total

php bin/phpstan analyze --debug bug-10772.php
  1.26s user 0.14s system 98% cpu 1.423 total

}

$errors = $this->runAnalyse(__DIR__ . '/data/bug-10979.php');
$this->assertCount(80, $errors);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please fix the example so that it's valid and does not report any errors? And make sure it still reproduces the performance problem.

I'm worrying that the number 80 is going to fluctuate and is going to fail quite often as we work on PHPStan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we now have a zero errors repro

@ondrejmirtes ondrejmirtes merged commit 0463255 into phpstan:1.11.x May 11, 2024
444 of 445 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants