Avoid duplicate loading of configuration files

[ajardin]

Description (*)

While I was investigating why some pages in back-office are extremely slow on my local environment, I found out that we are currently loading multiple times all layouts.xml files in the \Magento\Theme\Model\PageLayout\Config\Builder class.

Through this change, I propose to only load them one time by adding a new property to store the result and reuse it for the next calls. There is no structural change and the loading time is reduced by almost 40% on my local environment, as you can see with the screenshots from Blackfire below.

Previously:

Now:

Comparison:

Manual testing scenarios (*)

1. Log in to the back office.
2. Go to Content > Pages.
3. Look at the loading time... 😄

Please note that I'm using Docker for Mac with this environment, and the application is in developer mode with all caches activated. The latest version is used for both Docker and Magento.

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)

[magento-engcom-team]

Hi @ajardin. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[ajardin]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @ajardin. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @ajardin, here is your new Magento instance.
Admin access: https://pr-19585.instances.magento-community.engineering/admin
Login: admin Password: 123123q

[ajardin]

@magento-engcom-team give me 2.3-develop instance

[magento-engcom-team]

Hi @ajardin. Thank you for your request. I'm working on Magento 2.3-develop instance for you

[magento-engcom-team]

Hi @ajardin, here is your Magento instance.
Admin access: https://i-19585-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

[sivaschenko]

@ajardin thanks for the contribution. Please see the review notes. Also, please consider covering the changed functionality with tests

[ajardin]

Hi @sivaschenko, thanks for taking time to review my pull request!

About your suggestion on the test coverage, as I've only slightly rewrote an existing code covered by \Magento\Theme\Test\Unit\Model\PageLayout\Config\BuilderTest::testGetPageLayoutsConfig, which kind of test should I write?

[orlangur]

@ajardin a unit test which would check that files are accessed only once. An additional call to existing test can be added, I believe.

[orlangur]

@ajardin one more thing. Could you tell more which pages are slow and in which scenario? I grepped all the usages and this data seems to be cached properly, for example

 public function getAllOptions()
    {
        if (!$this->_options) {
            $this->_options = $this->pageLayoutBuilder->getPageLayoutsConfig()->toOptionArray();
            array_unshift($this->_options, ['value' => '', 'label' => __('No layout updates')]);
        }
        return $this->_options;
    }

[ajardin]

Sorry @sivaschenko, I misunderstood your comments about the test suite... I've updated the existing test to check that the registered themes are loaded only once.

@orlangur the scenario is described in the pull request description (log in to the back office and go to Content > Pages). All my application responds correctly except pages that contain grids. And by comparing with the Cache Management page, I discovered that the layouts configuration are loaded multiple times in \Magento\Theme\Model\PageLayout\Config\Builder. Even if the change won't affect applications running on production mode, the impact is significant on our local environments.

I'm still not fully satisfied by the loading time, as we are still have to wait more than 20s, but I think it can be a first quick win. What's your opinion on that?

[orlangur]

@ajardin is \Magento\Cms\Block\Adminhtml\Page\Grid::_prepareColumns a problematic method? I'm pretty sure that builder class is not a right place for caching considering all calls of getPageLayoutsConfig. Please add caching in place where the problem actually occurs. There could be even some unnecessary objects creation where could be used only on object.

[ajardin]

Do you mean that using a property to store the result of getFilesContent() between several calls is a caching system that shouldn't be part of the builder class?
Or should we implement another thing in \Magento\Cms\Block\Adminhtml\Page\Grid to properly use Magento cache when available, in addition to what I already did?

[orlangur]

@ajardin,

is a caching system that shouldn't be part of the builder class?

Yep. Caching already occurs outside of builder class in most cases.

[ajardin]

I think there is still a misunderstanding. 😄

My current approach is to avoid duplicate calls to the filesystem within the same builder class by using a property which will save the result after the initial call. There is currently no usage of the Magento cache.

For several reasons in fact:

• as you said, it's not the right place to manage Magento cache.
• as the issue only occurs in development mode, it could be worthless to rely only on Magento cache since it's often deactivated in this context.

There are two different things to address from my point of view. First the builder class can be improved to mitigate its impact on the overall performances because it's the most time consuming part. And then, we'll look at the grid class to check how to take advantage of Magento cache when it's possible.

[orlangur]

@ajardin by caching I mean storing in class property, but in another class, here was an example: #19585 (comment)

I would like to avoid storing data in property in multiple places, so, please find out where calls to builder are still not cached.

[sivaschenko]

@orlangur I don't think the cache of client classes are related to this contribution.

The Magento\Theme\Model\PageLayout\Config\Builder is loading and merging file contents from the filesystem and should cache the result to avoid multiple resource consuming operations.

[orlangur]

@sivaschenko I don't see any scenario where this configuration is read more than once. Let's wait for the reply from contributor.

There is no need to introduce any such internal caching unless really necessary because it is quite memory consuming.

[ajardin]

Sorry for the delay, but I thought the discussion was resolved after @sivaschenko's last comment.

@orlangur, there is already a "caching" system through to a property in \Magento\Cms\Model\Page\Source\PageLayout, but there is also an instance of \Magento\Cms\Model\Page\Source\CustomLayout. Since these two classes are calling directly the \Magento\Framework\View\Model\PageLayout\Config\BuilderInterface::getPageLayoutsConfig method, it makes their property inefficient to avoid duplicate calls to the filesystem.

Here is another screenshot from my initial Blackfire profile to illustrate this behavior.

[orlangur]

@ajardin thanks, so there are actually two calls made in different classes within same scenario.

What do you think about removing unneeded internal caching of options so that we consume less memory?

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3634 has been created to process this Pull Request

[sivaschenko]

@orlangur @ajardin removing protected properties is backward incompatible change. Such changes can break environments where these classes are extended and properties used.
Please keep the properties and assigned values (please mark them as deprecated if you don't think they are needed).

[orlangur]

@sivaschenko good point, thanks.

[ajardin]

Hi @orlangur, do you see something else to change?

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3634 has been created to process this Pull Request

[sivaschenko]

Hi @ajardin , thank you for your updates. Can you please keep the implementation of client methods as they are until the deprecated property will be removed.

[sivaschenko]

It looks a bit wired to set a value to the property and avoid using it. I think existing method body is more clear and avoids additional options processing.

[orlangur]

There is no any additional "option processing".

It looks a bit wired to set a value to the property and avoid using it

This is what current recommendation says. It would be much better to just remove property as probability of BC break is negligible, but if you insist on keeping it deprecated - let's just keep it prepared for removal.

[ajardin]

Hi guys, are you expecting something from me?

Because I made what was the most logical thing from my point of view:

• deprecate the property
• keep assigning a value to the deprecated property
• remove internal usages of the property to prepare its removal

But I'm not sure to understand exactly what you want right now... 😄

[orlangur]

@ajardin yeah, everything is fine from my point of view. Remaining part is to convince Sergey, no code changes required until we agree upon them.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3634 has been created to process this Pull Request

[ajardin]

Hi @orlangur and @sivaschenko,
Do you have any news about this pull request?

(Happy New Year by the way)

[sivaschenko]

Hi @ajardin , Happy New Year! This pull request is currently going throug internal verification process and is on it's way to mainline.

[ajardin]

Hi @sivaschenko, thanks for the confirmation!
I was asking this question as the pull request is still tagged as Progress: needs update. 😉

[sivaschenko]

Hi @ajardin sorry, and thanks for notification, the label was removed.

[ghost]

Hi @ajardin, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[magento-engcom-team]

Hi @ajardin. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.