Skip to content

Prevent unnecessary isSuperTypeOf() calls#2895

Merged
ondrejmirtes merged 1 commit intophpstan:1.10.xfrom
staabm:less-super
Jan 30, 2024
Merged

Prevent unnecessary isSuperTypeOf() calls#2895
ondrejmirtes merged 1 commit intophpstan:1.10.xfrom
staabm:less-super

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Jan 30, 2024

No description provided.

@ondrejmirtes ondrejmirtes merged commit 142dc2f into phpstan:1.10.x Jan 30, 2024
@ondrejmirtes
Copy link
Copy Markdown
Member

Thank you.

@staabm staabm deleted the less-super branch January 30, 2024 17:05
@thg2k
Copy link
Copy Markdown
Contributor

thg2k commented Feb 1, 2024

Does this change bring any measurable benefit? Looks like it just reduces the readability.

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Feb 1, 2024

I don't have a concrete repro case for the change.

My experience from watching at tens of profiles over the past is, that phpstan is CPU bound, mostly on Type method calls, especially isSuperTypeOf() which can have very different characteristics if e.g. big array types are involved.

@mad-briller
Copy link
Copy Markdown
Contributor

I've seen this pattern elsewhere in *Type classes too and considered this change but never sent a pr as i couldn't prove any benefits, because it is pretty hard to create repro cases for and the readability does suffer.

One thing i have investigated in the past is a compromise between the two using generators:

// IterableType.php

return $limit->and(
	$otherType->isIterable(),
	$otherType->getIterableValueType()->isSuperTypeOf($this->itemType),
	$otherType->getIterableKeyType()->isSuperTypeOf($this->keyType),
);

could become:

return $limit->yieldAnd(function() use ($otherType): Generator {
	yield $otherType->isIterable();
	yield $otherType->getIterableValueType()->isSuperTypeOf($this->itemType);
	yield $otherType->getIterableKeyType()->isSuperTypeOf($this->keyType);
});

This generator approach would allow complex logic to be done in a lazy way, including loops, conditionals etc. without suffering too much readability loss; but again because i could never come up with concrete gains so i never sent a pr.

@ondrejmirtes
Copy link
Copy Markdown
Member

There's already TrinaryLogic::lazyAnd() for that.

@mad-briller
Copy link
Copy Markdown
Contributor

yeah that was another reason i didn't send it 'cos having a third option would cause confusion aswell, however i have run into some limitations around lazyAnd that makes it awkward to use in some cases.

Not all cases you want to be lazy involve applying the same closure to a list of objects, for example how could you refactor the given example from IterableType to use lazyAnd()? i'm not sure you could

@ondrejmirtes
Copy link
Copy Markdown
Member

Yeah, you would save the isSuperTypeOf, that would be lazy, but you would not save the $otherType->isIterable() etc. calls first.

@thg2k
Copy link
Copy Markdown
Contributor

thg2k commented Feb 1, 2024

I'm in favor of cpu optimizations when they make sense, but here it really looks like we are gaining pennies and paying more in readability. A wise person once told me: Programmers time is more valuable than CPU time.

@ondrejmirtes
Copy link
Copy Markdown
Member

Programmers time is more valuable than CPU time

Yes, this holds true. That's why it's always worth it to make PHPStan faster, because it saves times for programmers all around the planet. (Not just PHPStan programmers, but a lot of other PHP programmers as well.)

@phpstan phpstan locked as resolved and limited conversation to collaborators Feb 1, 2024
@ondrejmirtes
Copy link
Copy Markdown
Member

Locking this - don't take this personally, but people always love to bikeshed the smallest of issues, instead of helping where the help would matter :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants