inline calls in TrinaryLogic to reduce method call overhead

[staabm]

looking at perf profiles of the slow test d33454d we can see TrinaryLogic creation is a major cost.

with this PR I propose to "duplicate" a few lines of code to reduce the callstack in this very perf critical path

with blackfire I can see a 30% perf improvement before/after this PR when running

blackfire run php vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserIntegrationTest.php --filter testBug5081

refs phpstan/phpstan#7666 (comment)

the perf improvement is also visible when running plain

time vendor/bin/phpunit tests/PHPStan/Analyser/AnalyserIntegrationTest.php

before this PR 15-17secs; after 13-15 secs

env

php -v
PHP 8.1.2 (cli) (built: Jul 21 2022 12:10:37) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2, Copyright (c), by Zend Technologies
    with blackfire v1.80.0~linux-x64-non_zts81, https://blackfire.io, by Blackfire

[herndlm]

This has such an impact although it was already implemented in create?

[staabm]

I think the impact is the actual call-overhead to create() not the logic implemented in the method. we call it milions of times and that adds up.

[oprypkhantc]

Just curious, can't PHPStan use native enums? That would likely reduce the overhead even more, although would require bumping up PHP requirement to 8.1.

[canvural]

Just curious, can't PHPStan use native enums? That would likely reduce the overhead even more, although would require bumping up PHP requirement to 8.1.

PHPStan's source code is downgraded with Rector in the PHAR build. And I guess Rector can't downgrade native enums. That's why.

[staabm]

after this change, the TrinaryLogic is no longer on the slow path. IMO its fine as is.

if we find a case where its still measurably slow, we can think about enums IMO.

this PR is about reducing function/method call overhead.
using enums would only help us in this regard if we would directly access the enum cases instead of invoking a method like we do atm with TrinaryLogic::createYes() etc

its only relevant for super huge union types which are rare.

[mad-briller]

just theory crafting here so feel free to tell me if i'm wrong

but if phpstan is caching and reusing the same instance for yes no and maybe, would it be worth it to somehow pre-create the 3 instances needed and then return those directly?

this would avoid potentially millions of null checks, of which there are 3 cases where the null check is actually relevant (the first one of each case)

[ondrejmirtes]

It's a question of when it should be pre-precreated. PHPStan internals are not only executed through bin/phpstan, there are also other usages, like executing test cases, and also usage of internals in 3rd party tools like Rector.

Maybe it could work if we initialize it as a side effect right before class TrinaryLogic declaration.

[ondrejmirtes]

I was tempted to merge this, but this looks for the problem in the wrong place.

Sure, if you call TrinaryLogic a billion times, optimization like that can help.

The problem is that the previous version didn't call TrinaryLogic a billion times, so this problem didn't exist.

We need to fix the root cause, and not treat the symptoms.

More about my findings here: phpstan/phpstan#7666 (comment)

[staabm]

I agree this one is not the fix for the root cause.

Still its a easy perf improvement for one of the most used primitives in phpstan, without a relevant cost/complexity increase.

IMO it should be considered for merge as it will improve performance for cases with big/huge union types

[ondrejmirtes]

Yeah, I don't see why not. Thanks!