From 7b15553bb31dd32a6f61d2ae096c4dc120bf5e9e Mon Sep 17 00:00:00 2001 From: Andrey Legayev Date: Fri, 10 May 2019 11:42:35 +0300 Subject: [PATCH] Fix layout xml and page layout caching issue on redis cluster under high load Bugs which were fixed: - $this->pageLayout was not checked after reading from cache, but was used as is - two cache items were used in once place instead of one (performance impact) Changes: - replace 2 cache items by 1 - it should improve performance - add "_MERGED" to cache key suffix to have compatibility with old cache keys during deployment of new version --- .../Framework/View/Layout/MergeTest.php | 22 ++++++++-- .../Framework/View/Model/Layout/Merge.php | 37 ++++++++++++---- .../View/Test/Unit/Model/Layout/MergeTest.php | 43 +++++++++++++++++++ 3 files changed, 91 insertions(+), 11 deletions(-) diff --git a/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php b/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php index 3ba43f7142e82..bac448f400992 100644 --- a/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php +++ b/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php @@ -40,6 +40,11 @@ class MergeTest extends \PHPUnit\Framework\TestCase */ protected $_cache; + /** + * @var \Magento\Framework\Serialize\SerializerInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $_serializer; + /** * @var \PHPUnit_Framework_MockObject_MockObject */ @@ -94,6 +99,8 @@ protected function setUp() $this->_cache = $this->getMockForAbstractClass(\Magento\Framework\Cache\FrontendInterface::class); + $this->_serializer = $this->getMockForAbstractClass(\Magento\Framework\Serialize\SerializerInterface::class); + $this->_theme = $this->createMock(\Magento\Theme\Model\Theme::class); $this->_theme->expects($this->any())->method('isPhysical')->will($this->returnValue(true)); $this->_theme->expects($this->any())->method('getArea')->will($this->returnValue('area')); @@ -129,6 +136,7 @@ function ($filename) use ($fileDriver) { 'resource' => $this->_resource, 'appState' => $this->_appState, 'cache' => $this->_cache, + 'serializer' => $this->_serializer, 'theme' => $this->_theme, 'validator' => $this->_layoutValidator, 'logger' => $this->_logger, @@ -264,9 +272,16 @@ public function testLoadFileSystemWithPageLayout() public function testLoadCache() { + $cacheValue = [ + "pageLayout" => "1column", + "layout" => self::FIXTURE_LAYOUT_XML + ]; + $this->_cache->expects($this->at(0))->method('load') - ->with('LAYOUT_area_STORE20_100c6a4ccd050e33acef0553f24ef399961') - ->will($this->returnValue(self::FIXTURE_LAYOUT_XML)); + ->with('LAYOUT_area_STORE20_100c6a4ccd050e33acef0553f24ef399961_page_layout_merged') + ->will($this->returnValue(json_encode($cacheValue))); + + $this->_serializer->expects($this->once())->method('unserialize')->willReturn($cacheValue); $this->assertEmpty($this->_model->getHandles()); $this->assertEmpty($this->_model->asString()); @@ -413,7 +428,8 @@ public function testLoadWithInvalidLayout() ->willThrowException(new \Exception('Layout is invalid.')); $suffix = md5(implode('|', $this->_model->getHandles())); - $cacheId = "LAYOUT_{$this->_theme->getArea()}_STORE{$this->scope->getId()}_{$this->_theme->getId()}{$suffix}"; + $cacheId = "LAYOUT_{$this->_theme->getArea()}_STORE{$this->scope->getId()}" + . "_{$this->_theme->getId()}{$suffix}_page_layout_merged"; $messages = $this->_layoutValidator->getMessages(); // Testing error message is logged with logger diff --git a/lib/internal/Magento/Framework/View/Model/Layout/Merge.php b/lib/internal/Magento/Framework/View/Model/Layout/Merge.php index 5be878720620b..7d0801e062495 100644 --- a/lib/internal/Magento/Framework/View/Model/Layout/Merge.php +++ b/lib/internal/Magento/Framework/View/Model/Layout/Merge.php @@ -5,10 +5,12 @@ */ namespace Magento\Framework\View\Model\Layout; +use Magento\Framework\App\ObjectManager; use Magento\Framework\App\State; use Magento\Framework\Config\Dom\ValidationException; use Magento\Framework\Filesystem\DriverPool; use Magento\Framework\Filesystem\File\ReadFactory; +use Magento\Framework\Serialize\SerializerInterface; use Magento\Framework\View\Model\Layout\Update\Validator; /** @@ -40,9 +42,15 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface /** * Cache id suffix for page layout + * @deprecated it was replaced by PAGE_LAYOUT_MERGED_CACHE_SUFFIX */ const PAGE_LAYOUT_CACHE_SUFFIX = 'page_layout'; + /** + * Cache id suffix for layout xml and page layout + */ + const PAGE_LAYOUT_MERGED_CACHE_SUFFIX = 'page_layout_merged'; + /** * @var \Magento\Framework\View\Design\ThemeInterface */ @@ -53,6 +61,11 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface */ private $scope; + /** + * @var SerializerInterface + */ + private $serializer; + /** * In-memory cache for loaded layout updates * @@ -168,6 +181,7 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface * @param ReadFactory $readFactory, * @param \Magento\Framework\View\Design\ThemeInterface $theme Non-injectable theme instance * @param string $cacheSuffix + * @param SerializerInterface|null $serializer * @SuppressWarnings(PHPMD.ExcessiveParameterList) */ public function __construct( @@ -181,7 +195,8 @@ public function __construct( \Psr\Log\LoggerInterface $logger, ReadFactory $readFactory, \Magento\Framework\View\Design\ThemeInterface $theme = null, - $cacheSuffix = '' + $cacheSuffix = '', + SerializerInterface $serializer = null ) { $this->theme = $theme ?: $design->getDesignTheme(); $this->scope = $scopeResolver->getScope(); @@ -193,6 +208,7 @@ public function __construct( $this->logger = $logger; $this->readFactory = $readFactory; $this->cacheSuffix = $cacheSuffix; + $this->serializer = $serializer ?: ObjectManager::getInstance()->get(SerializerInterface::class); } /** @@ -425,12 +441,12 @@ public function load($handles = []) $this->addHandle($handles); - $cacheId = $this->getCacheId(); - $cacheIdPageLayout = $cacheId . '_' . self::PAGE_LAYOUT_CACHE_SUFFIX; + $cacheId = $this->getCacheId() . '_' . self::PAGE_LAYOUT_MERGED_CACHE_SUFFIX; $result = $this->_loadCache($cacheId); - if ($result) { - $this->addUpdate($result); - $this->pageLayout = $this->_loadCache($cacheIdPageLayout); + if ($result !== false && $result !== null) { + $data = $this->serializer->unserialize($result); + $this->pageLayout = $data["pageLayout"]; + $this->addUpdate($data["layout"]); foreach ($this->getHandles() as $handle) { $this->allHandles[$handle] = $this->handleProcessed; } @@ -443,8 +459,13 @@ public function load($handles = []) $layout = $this->asString(); $this->_validateMergedLayout($cacheId, $layout); - $this->_saveCache($layout, $cacheId, $this->getHandles()); - $this->_saveCache((string)$this->pageLayout, $cacheIdPageLayout, $this->getHandles()); + + $data = [ + "pageLayout" => (string)$this->pageLayout, + "layout" => $layout + ]; + $this->_saveCache($this->serializer->serialize($data), $cacheId, $this->getHandles()); + return $this; } diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php index 0cb3254e818a3..fc7d799331107 100644 --- a/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php +++ b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php @@ -27,11 +27,21 @@ class MergeTest extends \PHPUnit\Framework\TestCase */ private $scope; + /** + * @var \Magento\Framework\Cache\FrontendInterface|\PHPUnit_Framework_MockObject_MockObject + */ + private $cache; + /** * @var \Magento\Framework\View\Model\Layout\Update\Validator|\PHPUnit_Framework_MockObject_MockObject */ private $layoutValidator; + /** + * @var \Magento\Framework\Serialize\SerializerInterface|\PHPUnit_Framework_MockObject_MockObject + */ + private $serializer; + /** * @var \Psr\Log\LoggerInterface|\PHPUnit_Framework_MockObject_MockObject */ @@ -47,10 +57,12 @@ protected function setUp() $this->objectManagerHelper = new ObjectManager($this); $this->scope = $this->getMockForAbstractClass(\Magento\Framework\Url\ScopeInterface::class); + $this->cache = $this->getMockForAbstractClass(\Magento\Framework\Cache\FrontendInterface::class); $this->layoutValidator = $this->getMockBuilder(\Magento\Framework\View\Model\Layout\Update\Validator::class) ->disableOriginalConstructor() ->getMock(); $this->logger = $this->getMockForAbstractClass(\Psr\Log\LoggerInterface::class); + $this->serializer = $this->getMockForAbstractClass(\Magento\Framework\Serialize\SerializerInterface::class); $this->appState = $this->getMockBuilder(\Magento\Framework\App\State::class) ->disableOriginalConstructor() ->getMock(); @@ -59,9 +71,11 @@ protected function setUp() \Magento\Framework\View\Model\Layout\Merge::class, [ 'scope' => $this->scope, + 'cache' => $this->cache, 'layoutValidator' => $this->layoutValidator, 'logger' => $this->logger, 'appState' => $this->appState, + 'serializer' => $this->serializer, ] ); } @@ -92,4 +106,33 @@ public function testValidateMergedLayoutThrowsException() $this->model->load(); } + + /** + * Test that merged layout is saved to cache if it wasn't cached before. + */ + public function testSaveToCache() + { + $this->scope->expects($this->once())->method('getId')->willReturn(1); + $this->cache->expects($this->once())->method('save'); + + $this->model->load(); + } + + /** + * Test that merged layout is not re-saved to cache when it was loaded from cache. + */ + public function testNoSaveToCacheWhenCachePresent() + { + $cacheValue = [ + "pageLayout" => "1column", + "layout" => "" + ]; + + $this->scope->expects($this->once())->method('getId')->willReturn(1); + $this->cache->expects($this->once())->method('load')->willReturn(json_encode($cacheValue)); + $this->serializer->expects($this->once())->method('unserialize')->willReturn($cacheValue); + $this->cache->expects($this->never())->method('save'); + + $this->model->load(); + } }