Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicate loading of configuration files #19585

Merged
merged 4 commits into from Jan 26, 2019

Conversation

@ajardin
Copy link
Member

ajardin commented Dec 5, 2018

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:
Blackfire screenshot

Now:
Blackfire screenshot

Comparison:
Blackfire screenshot

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

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 5, 2018

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

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 5, 2018

@magento-engcom-team give me test instance

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 5, 2018

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

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 5, 2018

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

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 5, 2018

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

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 5, 2018

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

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 5, 2018

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 sivaschenko self-assigned this Dec 5, 2018

@sivaschenko
Copy link
Contributor

sivaschenko left a comment

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

@ajardin ajardin force-pushed the ajardin:2.3-develop branch from 4027758 to 06292bf Dec 5, 2018

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 5, 2018

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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 6, 2018

@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 ajardin force-pushed the ajardin:2.3-develop branch from 06292bf to 2a7d068 Dec 6, 2018

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 6, 2018

@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.

@orlangur orlangur self-assigned this Dec 6, 2018

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 6, 2018

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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 6, 2018

@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

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 7, 2018

@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

This comment has been minimized.

Copy link
Contributor

sivaschenko commented Dec 7, 2018

@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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 11, 2018

@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.

@sivaschenko

This comment has been minimized.

Copy link
Contributor

sivaschenko commented Dec 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

orlangur commented Dec 12, 2018

@sivaschenko good point, thanks.

@ajardin ajardin force-pushed the ajardin:2.3-develop branch 2 times, most recently from 949cefe to 7cf42d6 Dec 12, 2018

@ajardin ajardin force-pushed the ajardin:2.3-develop branch from 7cf42d6 to 985a8a1 Dec 16, 2018

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Dec 16, 2018

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

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 17, 2018

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

@sivaschenko
Copy link
Contributor

sivaschenko left a comment

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.

*/
public function getAllOptions()
{
if (!$this->_options) {

This comment has been minimized.

@sivaschenko

sivaschenko Dec 17, 2018

Contributor

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.

This comment has been minimized.

@orlangur

orlangur Dec 17, 2018

Contributor

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.

This comment has been minimized.

@ajardin

ajardin Dec 17, 2018

Author Member

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... 馃槃

This comment has been minimized.

@orlangur

orlangur Dec 17, 2018

Contributor

@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

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 17, 2018

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

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Jan 7, 2019

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

(Happy New Year by the way)

@sivaschenko

This comment has been minimized.

Copy link
Contributor

sivaschenko commented Jan 7, 2019

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

@ajardin

This comment has been minimized.

Copy link
Member Author

ajardin commented Jan 17, 2019

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

@sivaschenko

This comment has been minimized.

Copy link
Contributor

sivaschenko commented Jan 18, 2019

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

@magento-engcom-team magento-engcom-team merged commit 38d1473 into magento:2.3-develop Jan 26, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@contribution-survey

This comment has been minimized.

Copy link

contribution-survey bot commented Jan 26, 2019

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 pushed a commit that referenced this pull request Jan 26, 2019

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Jan 26, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.