I was able to detect a performance penalty when using dozens of traits in even more classes. ThePhpStanExtractor creates aNameScope every time, but afaik this can be cached like inPhpDocExtractor (see#32188).

The performance impact is impressive, as it reduces the time needed for myTestCase by 30%. SeeBlackfire profile comparison.

mvhirsch requested a review fromdunglas as acode owner

May 12, 2024 15:43

carsonbot added Status: Needs Review PropertyInfo labels

May 12, 2024

carsonbot added this to the6.4 milestone

May 12, 2024

derrabus added the Performance label

May 13, 2024

Copy link

Member

derrabus commentedMay 13, 2024

Please target 7.1. We usually don't merge performance improvements into LTS branches.

derrabus modified the milestones:6.4,7.1

May 13, 2024

dunglas reviewed

May 13, 2024

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php OutdatedShow resolvedHide resolved

derrabus reviewed

May 13, 2024

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php OutdatedShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/PhpStan/NameScopeFactory.php OutdatedShow resolvedHide resolved

mvhirsch force-pushed thephpstan-extractor branch from978e580 to767d355Compare

May 13, 2024 09:22

mvhirsch requested review fromOskarStark,chalasr,jderusse,kbond,lyrixx,welcoMattic,xabbuh andyceruto ascode owners

May 13, 2024 09:22

mvhirsch changed the base branch from6.4 to7.1

May 13, 2024 09:22

mvhirsch requested review fromderrabus anddunglas

May 15, 2024 05:04

mvhirsch force-pushed thephpstan-extractor branch from4378f20 tode14980Compare

May 15, 2024 05:05

Copy link

Member

xabbuh commentedMay 15, 2024

Shouldn't we now also implement theResetInterface to prevent memory leaks?

xabbuh modified the milestones:7.1,7.2

May 15, 2024

Copy link

ContributorAuthor

mvhirsch commentedMay 15, 2024

Shouldn't we now also implement theResetInterface to prevent memory leaks?

If so, thePhpDocExtractor should implement it too, I guess.

Copy link

Member

nicolas-grekas commentedMay 17, 2024•
edited
Loading

implement the ResetInterface to prevent memory leaks?

If all we store is class metadata, we don't need a reset IMHO as the leak is upper bounded, and resetting might effect performance of long running kernels for little memory savings in the end.

nicolas-grekas reviewed

May 17, 2024

View reviewed changes

src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php Outdated

		returnnull;
		}

		$nameScope =$this->contexts[$class.$declaringClass] ??=$this->nameScopeFactory->create($class,$declaringClass);

Copy link

Member

nicolas-grekasMay 17, 2024

Choose a reason for hiding this comment

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

Suggested change

	$nameScope =$this->contexts[$class.$declaringClass] ??=$this->nameScopeFactory->create($class,$declaringClass);
	$nameScope =$this->contexts[$class.'/'.$declaringClass] ??=$this->nameScopeFactory->create($class,$declaringClass);

Copy link

Member

nicolas-grekasMay 17, 2024

Choose a reason for hiding this comment

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

would it make sense to move this inside the loop, right before the nested foreach?:

$nameScope ??=$this->contexts[$class.'/'.$declaringClass] ??=$this->nameScopeFactory->create($class,$declaringClass);

Copy link

ContributorAuthor

mvhirschMay 17, 2024

Choose a reason for hiding this comment

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

would it make sense to move this inside the loop, right before the nested foreach?:

You mean here?

symfony/src/Symfony/Component/PropertyInfo/Extractor/PhpStanExtractor.php

Line 127 inde14980

Sounds reasonable to me. It may make sense to skip creating one unless really needed (and since my change is all about re-using a created one, it hardly influences performance).