PreloadExplicitClassSymbolsFixer - introduction #4891

Nyholm · 2020-03-20T23:00:02Z

The idea is to help a preloader to find classes that are used by adding a bunch of class_exists in the file.

A typical preloader uses reflection. See Symfony.

This PR is a draft to give an idea what I want to do. Please review first commit to get the idea without all complex logic.

TODO

Add a better way to detect what classes we don't need to preload, ie classes that is found by reflection
Better find and include classes we do need to preload
Add a bunch of more tests

julienfalque · 2020-03-21T12:57:29Z

This looks like a good idea but I don't have all the background information required to understand it.

The idea is to help a preloader to find classes that are used by adding a bunch of class_exists in the file.

Can you elaborate a bit please?

Nyholm · 2020-03-21T13:02:32Z

Look at the Symfony preloader. It reads all public functions for their arguments and return types. It makes sure to preload them.

However, Preloaders can not find class App\Foo in the code below:

use App\Foo;

class Case003
{
    private $foo;
    public function __construct()
    {
        $this->foo = new Foo();
    }
}

That is why you need to help the preloader by loading the class Foo in runtime. That could easily be done by adding class_exists(Foo::class).

See this PR as an example for such work: symfony/symfony#36103

Nyholm · 2020-03-21T13:43:06Z

This is my first fixer.
It was a huge relief when I discovered all methods on Tokens and the FunctionAnalyzer.

I would appreciate some help with the FIXME comments. Is there an easy/common way to solve these issues or will I have to try to invent something myself?

Nyholm · 2020-03-21T13:47:55Z

tests/Fixtures/Preload/Case020_Out.php

+use App\Foo;
+
+class_exists('App\Foo');
+class_exists(Foo::class);


This is not a nice solution. It will duplicate the class_exists(Foo::class); for the App\Foo class. However, it is soo much more easier.

Allowing this would avoid parsing use statements and class alias to figure out that App\Foo is already "preloaded"

(...) and class alias (...)

I don't see class alias here in this test case, but if you need to handle them, maybe use https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.16/src/Tokenizer/Analyzer/NamespaceUsesAnalyzer.php ?

keradus · 2020-03-23T21:55:05Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+    public function getDefinition()
+    {
+        return new FixerDefinition(
+            'Adds extra `class_exists` before a class to help opcache.preload to discover always-needed symbols.',


Nice idea to make this as a fixer!

Please provide a $description parameter with the reasoning, especially:

why the rule is worth to use?

what classes we don't need to preload, ie classes that is found by reflection and why

I added a desciption

keradus · 2020-03-23T22:03:29Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+                $class = $tokens[$tokens->getPrevMeaningfulToken($i)]->getContent();
+                $classes[] = $class;
+            } elseif ($token->isGivenKind(T_OBJECT_OPERATOR)) {
+                // FIXME Better check if function call to avoid false positive like "$this->bar = 2;"


@Nyholm , are those 3 FIXME cases covered by tests already? if not, maybe we can add a test case for each, so we can easier understand what is example code snippet you struggle with and expected outcome ?

They are. They are the failing tests.
I'll work with them now to see if I can make sure they are green

Tests are green. Locally at least. =)

tests/Fixer/Preload/PreloadExplicitClassSymbolsFixerTest.php

.travis.yml

keradus · 2020-04-07T21:50:39Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+                    new VersionSpecification(70300)
+                ),
+            ],
+            'A script for opcache.preload should classes that are used by the average '


A script for opcache.preload should classes that are used by the average HTTP request.

should classes? sorry, i can't understand this sentence

keradus · 2020-04-07T21:51:28Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+            ],
+            'A script for opcache.preload should classes that are used by the average '
+            .'HTTP request. A good preloading script uses reflection to load the '
+            .'hot class and all classes it depends on. Reflection cannot always find '


what is hot class?
there is only ONE hot class, or many of them (classes)?

keradus · 2020-04-07T21:51:46Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+            'A script for opcache.preload should classes that are used by the average '
+            .'HTTP request. A good preloading script uses reflection to load the '
+            .'hot class and all classes it depends on. Reflection cannot always find '
+            .'all dependencies. Example: Classes used in the constructor or dependent '


Reflection cannot always find all dependencies

why?

(sorry for dummy questions, I am unaware about the idea and description here is not helping me to get what exactly should be added as outcome of this rule and why, and what not as it's automatically picked up by some preloading script)

keradus · 2020-04-07T21:54:13Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+        }
+
+        // TODO parse public properties
+        // TODO Get the class' interfaces and parent


marker
(so we don't merge before it's solved)

@Nyholm , would you like to finalize this PR?

keradus · 2020-04-07T21:56:15Z

src/Fixer/Preload/PreloadExplicitClassSymbolsFixer.php

+            $newTokens[] = new Token([CT::T_CLASS_CONSTANT, 'class']);
+            $newTokens[] = new Token(')');
+            $newTokens[] = new Token(';');
+            $newTokens[] = new Token([T_WHITESPACE, "\n"]);


Suggested change

$newTokens[] = new Token([T_WHITESPACE, "\n"]);

$newTokens[] = new Token([T_WHITESPACE, $this->whitespacesConfig->getLineEnding()]);

(you need to implement WhitespacesAwareFixerInterface also)

Wirone · 2023-07-13T21:14:22Z

Does anyone know if this PR is relevant in 2023 and PHP 8.2 (at the brink of 8.3)? @Nyholm, maybe @nicolas-grekas? 🤔

Nyholm · 2023-07-13T21:30:45Z

I was thinking of this pr the other day. Let’s close it.

If someone wants to pick it up, then feel free to open.

Nyholm force-pushed the preload branch from 74c0e80 to 7506b52 Compare March 21, 2020 11:44

julienfalque added the kind/feature label Mar 21, 2020

Nyholm force-pushed the preload branch from 6cf35f2 to ff61939 Compare March 21, 2020 13:40

Nyholm commented Mar 21, 2020

View reviewed changes

Nyholm force-pushed the preload branch 3 times, most recently from 6b1b0f0 to fd3ac34 Compare March 21, 2020 14:29

keradus reviewed Mar 23, 2020

View reviewed changes

tests/Fixer/Preload/PreloadExplicitClassSymbolsFixerTest.php Outdated Show resolved Hide resolved

keradus reviewed Mar 23, 2020

View reviewed changes

tests/Fixer/Preload/PreloadExplicitClassSymbolsFixerTest.php Outdated Show resolved Hide resolved

Nyholm force-pushed the preload branch from e4c7f9b to 792b0ce Compare April 4, 2020 17:35

Nyholm marked this pull request as ready for review April 4, 2020 18:02

Nyholm commented Apr 4, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

SpacePossum changed the title ~~Support for preload~~ PreloadExplicitClassSymbolsFixer - introduction Apr 5, 2020

Nyholm and others added 11 commits April 5, 2020 14:35

Adding basic idea of Preload fixer

d20bbe5

Added some more test cases

7c0f424

Adding some more logic

c854955

Parse other functions

c02fa8f

Parse public methods better

6b84302

Rename file, adding tests and updated readme

913d9fe

Exit early

fbf4490

minor

0981b18

rename files

3303dec

make folder meaningful

d340ad2

Remove debug code and fixed the tests

077a80d

Nyholm force-pushed the preload branch from d424444 to 077a80d Compare April 5, 2020 12:35

keradus reviewed Apr 7, 2020

View reviewed changes

Wirone added the status/to verify issue needs to be confirmed or analysed to continue label Jun 6, 2023

Wirone added the status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required label Jul 13, 2023

Nyholm closed this Jul 13, 2023

Wirone removed status/to verify issue needs to be confirmed or analysed to continue status/input required For the issue to be confirmed or the PR to be process; additional input of the author is required labels Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PreloadExplicitClassSymbolsFixer - introduction #4891

PreloadExplicitClassSymbolsFixer - introduction #4891

Nyholm commented Mar 20, 2020 •

edited

julienfalque commented Mar 21, 2020

Nyholm commented Mar 21, 2020

Nyholm commented Mar 21, 2020

Nyholm Mar 21, 2020

keradus Mar 23, 2020

keradus Mar 23, 2020

Nyholm Apr 4, 2020

keradus Mar 23, 2020

Nyholm Apr 4, 2020

Nyholm Apr 4, 2020

keradus Apr 7, 2020

keradus Apr 7, 2020

keradus Apr 7, 2020 •

edited

keradus Apr 7, 2020

keradus Oct 4, 2021

keradus Apr 7, 2020 •

edited

Wirone commented Jul 13, 2023

Nyholm commented Jul 13, 2023

	$newTokens[] = new Token([T_WHITESPACE, "\n"]);
	$newTokens[] = new Token([T_WHITESPACE, $this->whitespacesConfig->getLineEnding()]);

PreloadExplicitClassSymbolsFixer - introduction #4891

PreloadExplicitClassSymbolsFixer - introduction #4891

Conversation

Nyholm commented Mar 20, 2020 • edited

TODO

julienfalque commented Mar 21, 2020

Nyholm commented Mar 21, 2020

Nyholm commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus Apr 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus Apr 7, 2020 • edited

Choose a reason for hiding this comment

Wirone commented Jul 13, 2023

Nyholm commented Jul 13, 2023

Nyholm commented Mar 20, 2020 •

edited

keradus Apr 7, 2020 •

edited

keradus Apr 7, 2020 •

edited