Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

kschroeder · 2017-03-21T20:39:05Z

Reduces calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class.

Description

I have had issues with fairly slow setup:di:compile calls. In the course of my investigation, I ran the XDebug profiler and found that there were several million calls to SplFileInfo::realpath(). This change reduced my compile time from 9-10 minutes to 3 minutes.

[update]

I have been able to find several 10's of seconds (100+ seconds on my test box) of efficiency to be gained by creating a single extraction class that looks only for the first namespace/class match.

[update]

The single extraction class has been changed so that it now scans the entire PHP source file to find unconventionally defined classes as well

Manual testing scenarios

This test simply reduces the number of system calls on the filesystem. There is a functional change, not a logical change.

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

…ce in a loop instead of several times. This can have a significant performance impact on large codebases.or slow filesystems.

…velop

magento-cicd2 · 2017-03-21T20:39:06Z

All committers have signed the CLA.

piotrkwiecinski · 2017-03-21T21:23:01Z

@kschroeder nice one :) I've tested in on dev setup and for me it went down from 7 to 4 min and on CI server from 2:30 to 46 sec. Much appreciated you've saved quite a few CPU cycles :).

kschroeder · 2017-03-21T21:30:42Z

@piotrkwiecinski Sweet, I was hoping it wasn't just me! :-)

schmengler · 2017-03-21T21:47:31Z

Nice, verifiable PR with huge benefits! Unfortunately your change in ClassesScanner.php triggered the cyclomatic complexity check in the static test suite. This is not really a problem with your change, but rather with the original method. Let's take this as a chance to refactor the method a bit more. A simple extract method in the loop should do it.

kschroeder · 2017-03-21T21:58:02Z

@schmengler I updated the PR with a new commit that separates the extract() method. If this one doesn't pass I'll separate the $classNames loop.

schmengler · 2017-03-21T22:17:07Z

--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 59 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

…mments

PieterCappelle · 2017-03-22T12:42:01Z

The best community ever. Great PR.

…ce in a loop instead of several times. This can have a significant performance impact on large codebases.or slow filesystems.

…mments

… class names. It's like FileScanner except it only cares about namespaces and classes.

…velop

kschroeder · 2017-03-23T22:21:54Z

Note that I found another 2 minutes or so I could squeeze out of it, which is where those new commits are coming from. However, I found an edge case that is breaking it a little bit so it is not ready to be merged yet.

… word "class" was in a class. This change will now only return the Namespace\Class up until the first {. Given that there should only be one class per file this should have no effect beyond reducing the potential for problems.

…structures are scanned five times over the course of compilation. /var/generation is excluded.

…y so other unit tests wouldn't break. It just checks to see if /generation/ is in the filename,

schmengler · 2017-04-19T17:15:46Z

@maghamed the PR still needs your approval, please leave feedback. This is an important change and it would be a shame if we don't get it into the next release.

maghamed · 2017-04-21T17:17:55Z

setup/src/Magento/Setup/Module/Di/Code/Reader/ClassesScanner.php

+        if ($directoryList === null) {
+            $directoryList = ObjectManager::getInstance()->get(DirectoryList::class);
+        }
+        $this->generationDirectory = $directoryList->getPath(DirectoryList::GENERATION);


it's not allowed to make function calls in the constructor, just assignments to class variables.

here we receive $directoryList and after that make a call to get Path

maghamed · 2017-04-21T17:24:55Z

setup/src/Magento/Setup/Module/Di/Code/Reader/InvalidFileException.php

+
+namespace Magento\Setup\Module\Di\Code\Reader;
+
+class InvalidFileException extends \InvalidArgumentException


Custom Exceptions are not recommended to be introduced in each module.

Here is more discussion on this subject - https://github.com/magento/magento2/pull/9315/files#r112514206

For Magento\Setup\Module\Di\Code\Reader\FileClassScanner::__construct I'd suggest to go with \Magento\Framework\Exception\FileSystemException. It's used the same way in \Magento\Framework\Filesystem\Directory\Write::assertIsFile.

For Magento\Setup\Module\Di\Code\Reader\FileClassScanner::isBracedNamespace, I'm a bit undecided which of the existing exeptions is the best fit. Maybe \Magento\Framework\Exception\NotFoundException, at least for one of the two cases?

maghamed · 2017-04-21T17:25:20Z

setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Reader/ClassesScannerTest.php

+     *
+     * @var string
+     */
+


maghamed · 2017-04-21T17:42:33Z

setup/src/Magento/Setup/Module/Di/Code/Reader/FileClassScanner.php

+     * Warnings are suppressed for this method due to a micro-optimization that only really shows up when this logic
+     * is called several millions of times, which can happen quite easily with even moderately sized codebases.
+     *
+     * @SuppressWarnings(PHPMD.CyclomaticComplexity)


So I would like a little guidance on what you would prefer: reduce the individual method complexity or notate MD to ignore CC?

If reducing the method complexity has performance impact, I'd say go for optimized performance in this case and mark the class as ignored.

@schmengler Performance and Complexity are two different metrics. And in this specific case these metrics do not correlate. Because high Cyclomatic complexity doesn't lead to high performance and vice versa.

In Magento we have a rule, that any new code should not have SuppressWarnings for CouplingBetween objects and CyclomaticComplexity. Because it's from the beginning has a Technical Debt.

High CyclomaticComplexity often says that some functionality has not been fully covered with automated testing. Because it's too hard to cover all the possible conditional logic and exit points for this method.

Moreover such code is fragile and really hard to maintain in future.

Some time ago we even actively used CRAP metrics for this.

So, this method should be decomposed on some smaller once

maghamed · 2017-04-21T17:43:30Z

setup/src/Magento/Setup/Module/Di/Code/Reader/FileClassScanner.php

+                )
+            );
+        }
+        $this->filename = $filename;


magento constructors should contain just assigments, and no business logic allowed inside

@maghamed I'd like to argue that this is a valid use case according to 2.3.1 of the Technical Guidelines:

2.3.1. Constructor SHOULD throw an exception when validation of an argument has failed.

In the example for 2.3.1, an array of options is validated and an InvalidArgumentException is thrown if one of the objects in the array doesn't implement a particular interface.

In the given code the provided filename has to be an existing file so for me, this is a valid validation. Which alternative approach would you suggest?

kschroeder · 2017-04-21T17:57:23Z

Forget this. I'm done. I have about a thousand other things I need to be working on.

mzeis · 2017-04-23T17:32:42Z

@kschroeder Thank you for putting so much work into this pull request! I hope we still can get your changes into core because - as @schmengler said - it would be a shame to not have it in future Magento versions.

joshuaadickerson · 2017-05-22T06:52:17Z

@kschroeder I was about to jump to your defense on rewriting that method and I still think it should be accepted and then it can be reworked at any point in the future. Then I checked out the class and I looked at your argument. Taking a method and rewriting it to use internal (to the class) methods, even at 55 million calls, should not make any noticeable difference. I think all of those variables you're declaring in the extract() method are properties of the class. When you call extract(), they represent the state of that object. The class's only purpose is for that method (which is good).

I don't care to submit a PR or follow the coding guidelines, but I created a test of what it would look like and how that would affect performance: https://gist.github.com/joshuaadickerson/240f3633d99106fb7aed44a98a0ef0fd

It would be interesting to see what the difference between your class and my class are for the build that you said requires 55m calls.

mzeis · 2017-06-04T13:22:43Z

@maghamed Please can you have a look at the comments I gave on April 23rd? Thanks!

antonkril · 2017-06-20T17:34:06Z

The PR improves the state of the product. Let's merge.

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965 - fixed code style

kschroeder added 2 commits March 21, 2017 15:28

Updated the class scanner so it calls SplFileInfo::realpath() only on…

43c898f

…ce in a loop instead of several times. This can have a significant performance impact on large codebases.or slow filesystems.

Merge branch 'develop' of https://github.com/magento/magento2 into de…

f1e6e2d

…velop

schmengler self-assigned this Mar 21, 2017

schmengler added this to the March 2017 milestone Mar 21, 2017

schmengler added improvement Application Framework labels Mar 21, 2017

Updated the class scanner to reduce cyclomatic complexity.

acc587f

maghamed self-requested a review March 21, 2017 22:04

kschroeder added 2 commits March 21, 2017 17:37

Stupid cyclomatic complexity tests for stupid missing function doc co…

b127afa

…mments

Fixed some PHPDoc mixups

1f5de9d

kschroeder added 6 commits March 23, 2017 16:54

Updated the class scanner so it calls SplFileInfo::realpath() only on…

60a289b

…ce in a loop instead of several times. This can have a significant performance impact on large codebases.or slow filesystems.

Updated the class scanner to reduce cyclomatic complexity.

06865dc

Stupid cyclomatic complexity tests for stupid missing function doc co…

98828dc

…mments

Fixed some PHPDoc mixups

0c86dcd

Added a FileClassScanner which uses the tokenizer to extract only the…

60be236

… class names. It's like FileScanner except it only cares about namespaces and classes.

Merge branch 'develop' of https://github.com/magento/magento2 into de…

e46b0a9

…velop

kschroeder added 6 commits March 23, 2017 18:37

This includes a cache for class results because the individual class …

4aa7ce0

…structures are scanned five times over the course of compilation. /var/generation is excluded.

Fixed some mess detection issues. Removed the DirectoryList dependenc…

55a765f

…y so other unit tests wouldn't break. It just checks to see if /generation/ is in the filename,

Fixed mess detection issues

63927ba

Fixed mess detection issues

bf87fdb

Another run at MD and Cyc. Complexity.

dd3f4b5

kschroeder added 7 commits April 11, 2017 11:35

Changed visibility for includeClasses()

00a91db

Changed type check

81d82be

Changed visibility

1f55c6b

Changed some spaces

978ae40

Changed some spaces

d7e601b

Updated for MD

cc72590

Updated for MD

8f043c1

maghamed suggested changes Apr 21, 2017

View reviewed changes

okorshenko modified the milestones: April 2017, May 2017 May 9, 2017

mzeis self-assigned this May 17, 2017

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

antonkril approved these changes Jun 20, 2017

View reviewed changes

okorshenko self-assigned this Jun 20, 2017

magento-team merged commit 8f043c1 into magento:develop Jun 20, 2017

magento-team pushed a commit that referenced this pull request Jun 20, 2017

MAGETWO-66502: Reduce calls to SplFileInfo::realpath() in the Magento…

dda2477

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

magento-team pushed a commit that referenced this pull request Jun 20, 2017

MAGETWO-66502: Reduce calls to SplFileInfo::realpath() in the Magento…

f94c903

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965 - fixed code style

magento-team pushed a commit that referenced this pull request Jun 20, 2017

MAGETWO-66502: Reduce calls to SplFileInfo::realpath() in the Magento…

e68e8bb

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965 - fixed code style

magento-team pushed a commit that referenced this pull request Jun 20, 2017

MAGETWO-66502: Reduce calls to SplFileInfo::realpath() in the Magento…

4385418

…\Setup\Module\Di\Code\Reader\ClassesScanner class #8965 - fixed code style

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

kschroeder commented Mar 21, 2017 •

edited by okorshenko

magento-cicd2 commented Mar 21, 2017 •

edited

piotrkwiecinski commented Mar 21, 2017

kschroeder commented Mar 21, 2017

schmengler commented Mar 21, 2017

kschroeder commented Mar 21, 2017

schmengler commented Mar 21, 2017

PieterCappelle commented Mar 22, 2017

kschroeder commented Mar 23, 2017

schmengler commented Apr 19, 2017

maghamed Apr 21, 2017

maghamed Apr 21, 2017

mzeis Apr 23, 2017

maghamed Apr 21, 2017

maghamed Apr 21, 2017

maghamed Apr 21, 2017

mzeis Apr 23, 2017

kschroeder commented Apr 21, 2017

mzeis commented Apr 23, 2017

joshuaadickerson commented May 22, 2017

mzeis commented Jun 4, 2017

antonkril commented Jun 20, 2017


		namespace Magento\Setup\Module\Di\Code\Reader;

		class InvalidFileException extends \InvalidArgumentException

Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

Reduce calls to SplFileInfo::realpath() in the Magento\Setup\Module\Di\Code\Reader\ClassesScanner class #8965

Conversation

kschroeder commented Mar 21, 2017 • edited by okorshenko

Description

Manual testing scenarios

Contribution checklist

magento-cicd2 commented Mar 21, 2017 • edited

piotrkwiecinski commented Mar 21, 2017

kschroeder commented Mar 21, 2017

schmengler commented Mar 21, 2017

kschroeder commented Mar 21, 2017

schmengler commented Mar 21, 2017

PieterCappelle commented Mar 22, 2017

kschroeder commented Mar 23, 2017

schmengler commented Apr 19, 2017

maghamed Apr 21, 2017

Choose a reason for hiding this comment

maghamed Apr 21, 2017

Choose a reason for hiding this comment

mzeis Apr 23, 2017

Choose a reason for hiding this comment

maghamed Apr 21, 2017

Choose a reason for hiding this comment

maghamed Apr 21, 2017

Choose a reason for hiding this comment

maghamed Apr 21, 2017

Choose a reason for hiding this comment

mzeis Apr 23, 2017

Choose a reason for hiding this comment

kschroeder commented Apr 21, 2017

mzeis commented Apr 23, 2017

joshuaadickerson commented May 22, 2017

mzeis commented Jun 4, 2017

antonkril commented Jun 20, 2017

kschroeder commented Mar 21, 2017 •

edited by okorshenko

magento-cicd2 commented Mar 21, 2017 •

edited