Cache the results of isCacheable calls to improve performance #40112
Conversation
This function call is relatively expensive when $elements has the full layout in it. Especially the PageCache module is calling this function a lot from the following places: - Magento\PageCache\Model\Layout\LayoutPlugin - Magento\PageCache\Observer\ProcessLayoutRenderElement On my local machine I've seen the impact to be anywhere between 20-40 milliseconds, when the page has been loaded a few times to warm up cache (config, block, layout, etc...), but FPC is circumvented. We could also opt to move this to the PageCache module, but I think it is ok to optimize this within the Layout class within the framework.
|
Hi @igorwulff. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
@magento run all tests |
There was a problem hiding this comment.
✅ The change looks good to me.
The method you're improving, originally did not look like a good example of code, however.
I was considering whether ?bool wouldn't be better than bool|null but there's no functional difference. I'm personally more towards ?bool
| if ($blockName !== false && $this->structure->hasElement($blockName)) { | ||
| $cacheable = false; | ||
| break; | ||
| if (!isset($this->isCacheableCache)) { |
There was a problem hiding this comment.
Why not using use comparator here? $this->isCacheableCache === null
We are sure that isCacheableCache property exists, so we can avoid this check
|
Just tried this patch and when you have all caches enabled, it will broke my customer account pages and checkout pages, since it won't be able to properly fetch the customer's ID. Could also be my installation, but reverting this patch did solve the issues. |
This function call is relatively expensive when $elements has the full layout in it. Especially the PageCache module is calling this function a lot from the following places:
On my local machine I've seen the impact to be anywhere between 20-40 milliseconds, when the page has been loaded a few times to warm up cache (config, block, layout, etc...), but FPC is circumvented.
We could also opt to move this to the PageCache module, but I think it is ok to optimize this within the Layout class within the framework.
Description (*)
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)