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.

Sign up for GitHub

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

Jump to bottom

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
+836 −17

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 10, 2024 •
edited
Loading

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..

@staabm staabm marked this pull request as draft May 10, 2024 11:28
staabm
staabm commented May 10, 2024
View reviewed changes
src/Type/TypeCombinator.php
@@ -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 referenced this pull request May 10, 2024
@ondrejmirtes
ClassReflection - cache enum cases
Loading
Loading status checks…
39ce042
@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

@staabm staabm mentioned this pull request May 10, 2024
Closed
ondrejmirtes
ondrejmirtes requested changes May 10, 2024
View reviewed changes
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 👍

src/Type/ObjectType.php Outdated
@@ -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()

src/Type/ObjectType.php Outdated
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 •
edited
Loading

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

@staabm
Optimize performance when comparing big enum-case unions
Loading
Loading status checks…
3f0bc55 
cs

Update ObjectType.php
ondrejmirtes
ondrejmirtes requested changes May 10, 2024
View reviewed changes
tests/PHPStan/Analyser/AnalyserIntegrationTest.php Outdated
}

$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

@staabm
cleanup test
Loading
Loading status checks…
d82291f
@ondrejmirtes ondrejmirtes merged commit 0463255 into phpstan:1.11.x May 11, 2024
444 of 445 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the fix2 branch May 11, 2024 06:03
@staabm staabm mentioned this pull request May 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Larave11 makes parsing large Enum files considerably slower.
3 participants
@staabm @phpstan-bot @ondrejmirtes