Replace module dependency sort algorithm #11164

cmuench · 2017-09-30T21:33:43Z

The new algorithm replaces the old bubble sort with a new topological
sort. This eliminates 53.000 is_array and 35.000 array_unique calls.

Description

The sort logic is moved into a new Sorter class. In the main class a bunch of code
could be removed. The new logic is much more simple than the old one.

Manual testing scenarios

Add a new module dependency.
Circular reference check can be tested by adding a circular reference in modules.xml files.

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)

orlangur · 2017-09-30T21:41:57Z

For the reference: previous attempt to improve this sorting was performed in #10649

Did you do some benchmarking? Amount of is_array calls is not a proper metric for analysis.

cmuench · 2017-09-30T21:48:44Z

@orlangur Yes, you are right. I run "bin/magento" with blackfire. Blackfire shows a ~20% better performance overall.
The module order is called twice during the bootstrap. This results in 4ms per request. The new algorithm should run in ~1ms.

orlangur · 2017-09-30T21:48:49Z

And, before discussing a concrete implementation in details, what do you think about using an existing #10649 (comment) topological sorting implementation?

My main concern regarding current implementation is that it encapsulates pretty abstract algorithm in concrete application. While it would be nice to have some universal TopologicalGraphSorter and just use it in one particular place for now.

cmuench · 2017-09-30T21:56:09Z

OK. We can move the algorithm in a new class where we can add elements. Every element should have an "id" attribute which must be unique. Every element can have a "payload" which can have type "mixed". So we can reuse the logic in other parts of the system.

orlangur · 2017-09-30T22:19:14Z

Thanks for quick feedback!

What do you think about https://github.com/marcj/topsort.php? Not necessary to fully include the library, for example, we can borrow just ArraySort implementation.

Could you please evaluate this suggestion? In such case new Magento class will just prepare list of modules in correct format.

cmuench · 2017-10-01T05:35:24Z

I tested this yesterday with Anton during the Magento Hackathon Munich.
The lib works like intended. IMHO Anton likes an own implementation in the Magento framework. I will discuss this with him.

orlangur · 2017-10-01T08:16:47Z

Ok, thanks. We can have "own" implementation by wrapping a good one in some Magento Framework component by the way :)

dmanners · 2017-10-01T12:01:32Z

lib/internal/Magento/Framework/Module/ModuleList/Loader.php

     */
    public function __construct(
        Dom $converter,
        Parser $parser,
        ComponentRegistrarInterface $moduleRegistry,
-        DriverInterface $filesystemDriver
+        DriverInterface $filesystemDriver,
+        Sorter $sorter


In this case I think the Sorter parameter should be optional http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter

dmanners · 2017-10-01T12:02:52Z

lib/internal/Magento/Framework/Module/ModuleList/Sorter.php

+     * @param array $parentElements
+     * @throws \Exception
+     */
+    protected function processElement($element, $parentElements = [])


Could this be updated to private since I think it is only used inside this class?

dmanners · 2017-10-01T12:04:56Z

lib/internal/Magento/Framework/Module/ModuleList/Sorter.php

@@ -0,0 +1,83 @@
+<?php
+/**
+ * @copyright Copyright (c) 1999-2017 netz98 GmbH (http://www.netz98.de)


I assume this is a php storm autocomplete. I guess this should be the Magento docblock instead.

:-) Yes, you catched me. I will change the PhpStorm template.

orlangur · 2017-10-02T16:30:03Z

@dmanners @cmuench just to clear the things up - we are sticking to own refactored implementation, not going to intergrate existed third-party toposort implementation?

cmuench · 2017-10-02T16:45:36Z

@orlangur Yes, that is the plan. Anton is not to be a fan of too many 3rd party integrations.

orlangur · 2017-10-02T17:27:00Z

Ok, and I'm not a big fan of reinventing the wheels when we can easily wrap and reuse something :)

As this logic is just a part of CLI and I'm not sure it is covered by any performance test, could you do a little benchmark? No need in performance boost, just to make sure there is no accidental degradation.

This eliminates 53.000 is_array and 35.000 array_unique calls.

I believe you already did some counting, just do timer start in the beginning and timer stop in the very end. Will do a full review soon.

orlangur · 2017-10-02T17:28:32Z

lib/internal/Magento/Framework/Module/ModuleList/Loader.php

@@ -97,7 +105,7 @@ public function load(array $exclude = [])
                $result[$name] = $data[$name];
            }
        }
-        return $this->sortBySequence($result);
+        return $this->sorter->sort($result);


Something like SequenceSorter seems to be a more appropriate name (both class and injected argument).

orlangur · 2017-10-02T17:28:55Z

lib/internal/Magento/Framework/Module/ModuleList/Sorter.php

+    /**
+     * @var array
+     */
+    private $origList;


orig -> original

orlangur · 2017-10-02T17:31:12Z

lib/internal/Magento/Framework/Module/ModuleList/Sorter.php

+     * @param array $origList
+     * @return array
+     */
+    public function sort(array $origList)


Please specify in PHPDoc of this method exact algorithm used.

orlangur · 2017-10-02T17:33:28Z

lib/internal/Magento/Framework/Module/ModuleList/Sorter.php

+    protected function processElement($element, $parentElements = [])
+    {
+        if (isset($parentElements[$element['name']])) {
+            $relatedName = implode('', array_slice(array_keys($parentElements), -1, 1));


This is quite ineffective operation. Looks like you needed https://stackoverflow.com/a/2348213/8640867

orlangur · 2017-10-02T17:36:20Z

lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/LoaderTest.php

@@ -54,7 +55,9 @@ protected function setUp()
        $this->parser->expects($this->once())->method('initErrorHandler');
        $this->registry = $this->createMock(\Magento\Framework\Component\ComponentRegistrarInterface::class);
        $this->driver = $this->createMock(\Magento\Framework\Filesystem\DriverInterface::class);
-        $this->loader = new Loader($this->converter, $this->parser, $this->registry, $this->driver);
+
+        $sorter = new Sorter();


We cannot test two classes in unit test. Create a new test specifically for SequenceSorter and remove anything unrelated from this one.

UPD: great, you already created such test 👍 Just mock SequenceSorter here then.

orlangur · 2017-10-02T17:39:05Z

lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/SorterTest.php

+use Magento\Framework\Module\ModuleList\Sorter;
+use PHPUnit\Framework\TestCase;
+
+class SorterTest extends TestCase


Please make sure new class is 100% covered. At least there is no test for Circular sequence reference from exception currently.

The new algorithm replace the old bubble sort with a new topological sort. This eliminates 53.000 is_array and 35.000 array_unique calls.

orlangur · 2017-10-30T09:21:13Z

Hi @cmuench, any updates on this one?

orlangur · 2017-11-10T12:20:58Z

Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on pull request and it will be reopened.

orlangur self-assigned this Sep 30, 2017

orlangur added the Progress: needs update label Sep 30, 2017

dmanners added the MageHackMuc17 label Oct 1, 2017

dmanners reviewed Oct 1, 2017

View reviewed changes

orlangur requested changes Oct 2, 2017

View reviewed changes

vkublytskyi added the partner contribution label Oct 3, 2017

Replace module dependency sort algorithm

2478d6f

The new algorithm replace the old bubble sort with a new topological sort. This eliminates 53.000 is_array and 35.000 array_unique calls.

orlangur added this to the November 2017 milestone Nov 10, 2017

orlangur closed this Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace module dependency sort algorithm #11164

Replace module dependency sort algorithm #11164

cmuench commented Sep 30, 2017 •

edited

Loading

orlangur commented Sep 30, 2017

cmuench commented Sep 30, 2017

orlangur commented Sep 30, 2017

cmuench commented Sep 30, 2017

orlangur commented Sep 30, 2017

cmuench commented Oct 1, 2017

orlangur commented Oct 1, 2017

dmanners Oct 1, 2017

dmanners Oct 1, 2017

dmanners Oct 1, 2017

cmuench Oct 2, 2017

orlangur commented Oct 2, 2017

cmuench commented Oct 2, 2017 •

edited

Loading

orlangur commented Oct 2, 2017

orlangur Oct 2, 2017

orlangur Oct 2, 2017

orlangur Oct 2, 2017

orlangur Oct 2, 2017

orlangur Oct 2, 2017

orlangur Oct 2, 2017

orlangur commented Oct 30, 2017

orlangur commented Nov 10, 2017

Replace module dependency sort algorithm #11164

Replace module dependency sort algorithm #11164

Conversation

cmuench commented Sep 30, 2017 • edited Loading

Description

Manual testing scenarios

Contribution checklist

orlangur commented Sep 30, 2017

cmuench commented Sep 30, 2017

orlangur commented Sep 30, 2017

cmuench commented Sep 30, 2017

orlangur commented Sep 30, 2017

cmuench commented Oct 1, 2017

orlangur commented Oct 1, 2017

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

orlangur commented Oct 2, 2017

cmuench commented Oct 2, 2017 • edited Loading

orlangur commented Oct 2, 2017

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

orlangur commented Oct 30, 2017

orlangur commented Nov 10, 2017

cmuench commented Sep 30, 2017 •

edited

Loading

cmuench commented Oct 2, 2017 •

edited

Loading