NameImportingPostRector is slow

[staabm]

Bug Report

Subject	Details
Rector version	e.g. v0.17.6

when $rectorConfig->importNames(); is enabled running rector across the otherwise identical codebase is 42% slower.

is it a known problem that $rectorConfig->importNames() is such a performance hit?

do we actually need $rectorConfig->importNames() in our projects when we run ECS right after we ran rector?

Minimal PHP Code Causing Issue

my rector command on the kunzmann private repo:

blackfire run php vendor/bin/rector process app/admin --ansi --clear-cache

config

$rectorConfig->importNames();
$rectorConfig->disableParallel();

Expected Behaviour

fast CI :)

[staabm]

another thing which makes me wonder: my rector run does not yield any required changes but it seems name importing is still slow?

sounds like rector does re-print files, even if no changes occurred?

[samsonasik]

ecs is only sorting use statements iirc, not make auto imports.

Some rules require print in the rule itself, like curly variable check {$f}, that probably happen.

Auto imports is expected to be slower, as it check every Name node already in use statement and its a complex one, as it also verify in group use and aliased.

[staabm]

further investigation: it seems the static-type-mapping is the slowest part

[staabm]

@TomasVotruba do you think we can expect this type-mapping will go away when #7959 is resolved?

[staabm]

Auto imports is expected to be slower, as it check every Name node already in use statement and its a complex one, as it also verify in group use and aliased.

@samsonasik do you think it would work to run the auto-import after all rectors have been applied instead of mid-flight as a post rector between rules?

in our case importNames() runs ~25.000 times for ~450 files

[staabm]

said differently: I have the feeling out of the default post-rectors

https://github.com/rectorphp/rector-src/blob/ff74e1000b87a53131af5957b69a635f6040ccc1/packages/PostRector/Application/PostFileProcessor.php#L37-L46

the NameImportingPostRector and UnusedImportRemovingPostRector rectors are those doing just "cosmetics" which are nice for the human reading the code, but should not be required for the rector rules themselfs (and therefore it could be enough to run them once per file at the very end)?

[samsonasik]

PostRector is already run after all rules applied to the file object:

rector/src/Application/FileProcessor/PhpFileProcessor.php

Lines 108 to 110 in 7f18bf8

	$this->fileProcessor->refactor($file);
	// 3. apply post rectors
	$newStmts = $this->postFileProcessor->traverse($file->getNewStmts());

It is in loop of while file still changed until no longer changed.

[staabm]

ecs is only sorting use statements iirc, not make auto imports.

maybe I did not yet understand what a "auto import" is.

In my understanding it means adding "use statements" for symbols which are fully-qualified within the source?
if so, maybe we could say this is a task for a code-style fixer (add this task to ECS) and remove it completly from rector?
same for removing unused imports.

It is in loop of while file still changed until no longer changed.

yeah. my question is, whether we can move it after the loop, as it might not be necessary todo it within it?
(this does not apply for all post rectors but maybe for NameImportingPostRector and UnusedImportRemovingPostRector ?)

[samsonasik]

Apply after loop is probably better, but that will make the Name node doesn't have target type early, eg: AliasedObjectType that maybe used on next rule, that make rector require double run to apply.

We basically don't want to run multiple times, except it happen on consecutive rule execution, or reprint that require space change, that php-parser doesn't have information immediatelly after change the node.