Make callbacks marked as static #6695

SCIF · 2021-10-19T21:55:21Z

Some callbacks converted to callables

src/Psalm/Internal/Codebase/Scanner.php

src/Psalm/Internal/Codebase/TaintFlowGraph.php

muglug · 2021-10-24T22:59:56Z

Is there a performance benefit from this?

SCIF · 2021-10-25T20:25:26Z

Is there a performance benefit from this?

Don't think so (hopefully, I'm wrong :)). It's more about memory leak prevention. Analyzing Psalm codebase shows about 0.5Mb of memory save. So it's more about a proper usage of callbacks rather than optimization.

sasezaki · 2021-10-30T03:27:14Z

@SCIF
Did you use any tool to convert ?

orklah · 2021-10-30T07:23:09Z

I believe PHPInspectionsEA for PHPStorm has a similar feature

SCIF · 2021-10-30T08:08:51Z

@SCIF Did you use any tool to convert ?

I've done it using regex replace :) I didn't try to find a special tool to do that

sasezaki · 2021-10-30T09:33:51Z

I believe PHPInspectionsEA for PHPStorm has a similar feature

I thought PHPInspectionsEA too :)

I've done it using regex replace :) I didn't try to find a special tool to do that

Ok, I see.
( If any stabilized tool available, we can include it for CI)

staabm · 2021-11-06T07:38:27Z

Afair php-cs-fixer can do it

orklah · 2022-01-20T20:17:04Z

If you can rebase this and @weirdan is okay with it, I'm up for merging this

SCIF · 2022-01-20T20:33:31Z

If you can rebase this and @weirdan is okay with it, I'm up for merging this

Will do today. Thanks!

SCIF · 2022-01-23T23:07:41Z

Sorry, I'm still in process of doing that. I'm fixing the conflicts because of replacing function () callbacks with arrow/fn().

SCIF · 2022-02-12T22:14:35Z

Guys, I just noticed there are A LOT of array_map() calls with passed callbacks. I was curious if the reason of usage is a speed. Made a simple test:

$arr = array_fill(0, 1_000_000, 'a');
$result = [];

$start = microtime(true);
foreach ($arr as $a) {
    $result[] = $a . '!';
}
echo ((microtime(true) - $start)*1000)."\n";

$start = microtime(true);
$result = array_map(static function(string $a): string {return $a.'!';}, $arr);

echo ((microtime(true) - $start)*1000)."\n";

$start = microtime(true);
$result = array_map(static fn(string $a): string =>  $a.'!', $arr);

echo ((microtime(true) - $start)*1000)."\n";

I was quite amazed to see that foreach() is MUCH faster. It's also much readable for cases when a multiline callback is passed to array_map() so I will rework a few places to use foreach()es

SCIF · 2022-02-12T22:56:32Z

All green apart from CS and labels. Will fix cs soon. Make this PR ready to review. Gonna squash commits prior merging, so let me know when it will look fine for everybody.

src/Psalm/Config.php

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArrayFunctionArgumentsAnalyzer.php

orklah · 2022-02-12T23:09:44Z

...Psalm/Internal/Analyzer/Statements/Expression/Call/StaticMethod/AtomicStaticCallAnalyzer.php

@@ -445,7 +445,7 @@ private static function handleNamedCall(

                    $mixin_candidates_no_generic = array_filter(
                        $mixin_candidates,
-                        fn($check): bool => !($check instanceof TGenericObject)
+                        static fn($check): bool => !($check instanceof TGenericObject)


type is known here

src/Psalm/Internal/Codebase/Analyzer.php

src/Psalm/Config.php

src/Psalm/Internal/Codebase/Scanner.php

src/Psalm/Config.php

SCIF · 2022-02-19T10:57:05Z

End-to-end failures looks valid to me:

ERROR: RedundantCast - src/Event/Value/TestSuite/TestSuite.php:128:21 - Redundant cast to int (see https://psalm.dev/262)
                    (int) $e->getCode(),

https://www.php.net/manual/en/throwable.getcode.php

orklah · 2022-02-19T11:19:39Z

End-to-end failures looks valid to me:

They are, but they doesn't come from your changes, we have to baseline them. @weirdan would you mind? I think we have some on 4.x as weel as on master

SCIF · 2022-02-19T22:37:11Z

Squashed the commits in the branch. Fixed all the comments.

I spoofed array_map() with my implementation and has collected the hot spots of using array_map(). Top several spots got me around 100k of array_map() calls. I reworked them into foreach(). Some other places were reworked because code just made much readable when used foreach (less indentation, no 3-5 uses needed). Reworking of Config class helped to get rid of 30k of array_map() calls as there is no need to have array_map() in a getter if a setter ensures the classes are lowercased.

Based on my app, all those changes speed it up 0.5-1s (4 threads, ~55 second in total), but it's really hard to evaluate the precise number as results fluctuate a lot. So I believe there is some speedup but it's actually not such noticeable.

src/Psalm/Internal/Algebra.php

src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php

src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php

src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php

src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php.orig

src/Psalm/Internal/Codebase/TaintFlowGraph.php.orig

src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php.orig

SCIF · 2022-03-25T22:31:02Z

src/Psalm/Internal/Algebra.php

                    }
+                }
+


I tried to debug errors:

ERROR: Trace - src/Psalm/Internal/Algebra.php:48:49 - $anded_types: non-empty-list<non-empty-list<Psalm\Storage\Assertion>> (see https://psalm.dev/224) /** @psalm-trace $anded_types */; ERROR: Trace - src/Psalm/Internal/Algebra.php:49:53 - $new_anded_types: list<Psalm\Storage\Assertion> (see https://psalm.dev/224) /** @psalm-trace $new_anded_types */;

It looks like continue 2; is not properly handled by psalm here.

I'd have to take a closer look but I don't have much time these days. Can you add an assert($new_anded_types !== []); ? I think this should do the trick :)

Yep, that helps. But that's excessive code (a performance waster) intended just to calm psalm. Tbh, adding this to baseline would be a bit better. I propose to wait a few days other suggestions :)

SCIF · 2022-03-25T22:51:39Z

src/Psalm/Internal/LanguageServer/LanguageServer.php

@@ -121,7 +121,7 @@ public function __construct(
        $this->protocolReader = $reader;
        $this->protocolReader->on(
            'close',
-            function (): void {
+            static function (): void {


This must fail. Probably the LanguageServer is not covered with tests.

SCIF · 2022-03-25T22:53:28Z

src/Psalm/Internal/LanguageServer/ProtocolStreamReader.php

@@ -56,7 +56,7 @@ public function __construct($input)
            /**
             * @return Generator<int, Promise<?string>, ?string, void>
             */
-            function () use ($input): Generator {
+            static function () use ($input): Generator {


This must fail as well.

SCIF · 2022-03-25T23:29:40Z

LanguageServer and ProtocolStreamReader apparently not covered with tests as my changes should have fail them. As I don't use this feature and don't know way to proper test it — I reverted back my changes as it would be too dangerous and not responsible.

SCIF · 2022-03-25T23:36:28Z

I squashed commits, double checked my changes, reverted too dangerous changes. Please give me a hand to overcome psalm errors I highlighted above. Would highly appreciate if you would review/help to conclude this PR as it's really time consuming (I know this is unwanted refactoring and you could oppose it, then I will considering closing the PR).

orklah · 2022-03-26T10:02:57Z

(I know this is unwanted refactoring and you could oppose it, then I will considering closing the PR)

I fully intend to merge this :) I haven't been able to spend a lot of time on Psalm these days, sorry if I don't always respond in time

SCIF · 2022-03-26T10:14:29Z

sorry if I don't always respond in time

No need to be sorry. You are one of the most active contributors and in general Psalm has very good response time which a lot of OpenSource projects can dream of.

SCIF · 2022-04-07T22:59:24Z

Don't want to bother by mentioning core devs, so just calling for the advice how to overcome the issue above. Gonna to add asserts over the weekend if no better solutions will be suggestion.

SCIF · 2022-04-09T10:33:56Z

I have merged the PR with the latest master. The issues coming from it.

orklah · 2022-04-09T10:51:21Z

I'm ready to merge if you are :)

SCIF · 2022-04-09T10:56:34Z

I'm ready to merge if you are :)

I did my best and double checked the logic of all changes. The places which I suspected, I reworked in a separate file and compared with my PR's one. That strategy gave me results I reflected in comments above. Looks fine to me now :)

A review from @weirdan is locking the merge apparently.

orklah · 2022-04-09T11:16:30Z

A review from @weirdan is locking the merge apparently.

nah, that's okay :)

Thanks!

weirdan reviewed Oct 20, 2021

View reviewed changes

src/Psalm/Internal/Codebase/Scanner.php Outdated Show resolved Hide resolved

src/Psalm/Internal/Codebase/TaintFlowGraph.php Outdated Show resolved Hide resolved

orklah marked this pull request as draft November 24, 2021 19:08

orklah added the PR: Needs work label Jan 20, 2022

SCIF marked this pull request as ready for review February 12, 2022 22:56

weirdan added the release:internal The PR will be included in 'Internal changes' section of the release notes label Feb 12, 2022

orklah reviewed Feb 12, 2022

View reviewed changes

src/Psalm/Config.php Show resolved Hide resolved

orklah reviewed Feb 12, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php Outdated Show resolved Hide resolved

orklah reviewed Feb 12, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArrayFunctionArgumentsAnalyzer.php Outdated Show resolved Hide resolved

orklah reviewed Feb 12, 2022

View reviewed changes

src/Psalm/Internal/Codebase/Analyzer.php Outdated Show resolved Hide resolved

weirdan requested changes Feb 12, 2022

View reviewed changes

src/Psalm/Config.php Show resolved Hide resolved

src/Psalm/Internal/Codebase/Scanner.php Outdated Show resolved Hide resolved

SCIF commented Feb 19, 2022

View reviewed changes

src/Psalm/Config.php Show resolved Hide resolved

SCIF force-pushed the make-callbacks-static branch from 327d408 to 67c329a Compare February 19, 2022 22:20

orklah added PR: Need review and removed PR: Needs work labels Feb 19, 2022

SCIF force-pushed the make-callbacks-static branch from 0c83c16 to ec35b06 Compare February 20, 2022 09:02

SCIF commented Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Algebra.php Outdated Show resolved Hide resolved

SCIF commented Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php Outdated Show resolved Hide resolved

SCIF commented Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php Show resolved Hide resolved

SCIF commented Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php Outdated Show resolved Hide resolved

weirdan reviewed Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php.orig Outdated Show resolved Hide resolved

weirdan reviewed Feb 20, 2022

View reviewed changes

src/Psalm/Internal/Codebase/TaintFlowGraph.php.orig Outdated Show resolved Hide resolved

weirdan reviewed Feb 20, 2022

View reviewed changes

src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php.orig Outdated Show resolved Hide resolved

orklah added PR: Needs work and removed PR: Need review labels Mar 25, 2022

SCIF force-pushed the make-callbacks-static branch 2 times, most recently from 5be3195 to 6baab4e Compare March 25, 2022 22:27

SCIF commented Mar 25, 2022

View reviewed changes

SCIF force-pushed the make-callbacks-static branch from 6baab4e to 3619194 Compare March 25, 2022 23:27

Made most of callbacks static. Reworked some array_map() into foreach()

b4fdc3e

SCIF force-pushed the make-callbacks-static branch from 3619194 to b4fdc3e Compare April 9, 2022 09:58

SCIF requested a review from weirdan April 9, 2022 10:51

orklah merged commit 916fddb into vimeo:master Apr 9, 2022

SCIF deleted the make-callbacks-static branch April 9, 2022 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make callbacks marked as static #6695

Make callbacks marked as static #6695

SCIF commented Oct 19, 2021

muglug commented Oct 24, 2021

SCIF commented Oct 25, 2021

sasezaki commented Oct 30, 2021

orklah commented Oct 30, 2021

SCIF commented Oct 30, 2021

sasezaki commented Oct 30, 2021

staabm commented Nov 6, 2021

orklah commented Jan 20, 2022

SCIF commented Jan 20, 2022

SCIF commented Jan 23, 2022

SCIF commented Feb 12, 2022

SCIF commented Feb 12, 2022

orklah Feb 12, 2022

SCIF commented Feb 19, 2022 •

edited

orklah commented Feb 19, 2022

SCIF commented Feb 19, 2022 •

edited

SCIF Mar 25, 2022

orklah Mar 26, 2022

SCIF Mar 26, 2022

SCIF Mar 25, 2022

SCIF Mar 25, 2022

SCIF commented Mar 25, 2022

SCIF commented Mar 25, 2022 •

edited

orklah commented Mar 26, 2022

SCIF commented Mar 26, 2022

SCIF commented Apr 7, 2022

SCIF commented Apr 9, 2022

orklah commented Apr 9, 2022

SCIF commented Apr 9, 2022

orklah commented Apr 9, 2022

Make callbacks marked as static #6695

Make callbacks marked as static #6695

Conversation

SCIF commented Oct 19, 2021

muglug commented Oct 24, 2021

SCIF commented Oct 25, 2021

sasezaki commented Oct 30, 2021

orklah commented Oct 30, 2021

SCIF commented Oct 30, 2021

sasezaki commented Oct 30, 2021

staabm commented Nov 6, 2021

orklah commented Jan 20, 2022

SCIF commented Jan 20, 2022

SCIF commented Jan 23, 2022

SCIF commented Feb 12, 2022

SCIF commented Feb 12, 2022

orklah Feb 12, 2022

Choose a reason for hiding this comment

SCIF commented Feb 19, 2022 • edited

orklah commented Feb 19, 2022

SCIF commented Feb 19, 2022 • edited

SCIF Mar 25, 2022

Choose a reason for hiding this comment

orklah Mar 26, 2022

Choose a reason for hiding this comment

SCIF Mar 26, 2022

Choose a reason for hiding this comment

SCIF Mar 25, 2022

Choose a reason for hiding this comment

SCIF Mar 25, 2022

Choose a reason for hiding this comment

SCIF commented Mar 25, 2022

SCIF commented Mar 25, 2022 • edited

orklah commented Mar 26, 2022

SCIF commented Mar 26, 2022

SCIF commented Apr 7, 2022

SCIF commented Apr 9, 2022

orklah commented Apr 9, 2022

SCIF commented Apr 9, 2022

orklah commented Apr 9, 2022

SCIF commented Feb 19, 2022 •

edited

SCIF commented Feb 19, 2022 •

edited

SCIF commented Mar 25, 2022 •

edited