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

Author: andrey-legayev

dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php

@@ -41,6 +41,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
      */
@@ -100,6 +105,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'));
@@ -140,6 +147,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,
@@ -276,9 +284,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());
@@ -425,7 +440,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

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\Layout\LayoutCacheKeyInterface;
 use Magento\Framework\View\Model\Layout\Update\Validator;
 
@@ -42,7 +44,7 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface
     /**
      * Cache id suffix for page layout
      */
-    const PAGE_LAYOUT_CACHE_SUFFIX = 'page_layout';
+    const PAGE_LAYOUT_CACHE_SUFFIX = 'page_layout_merged';
 
     /**
      * @var \Magento\Framework\View\Design\ThemeInterface
@@ -54,6 +56,11 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface
      */
     private $scope;
 
+    /**
+     * @var SerializerInterface
+     */
+    private $serializer;
+
     /**
      * In-memory cache for loaded layout updates
      *
@@ -173,10 +180,11 @@ class Merge implements \Magento\Framework\View\Layout\ProcessorInterface
      * @param \Magento\Framework\Cache\FrontendInterface $cache
      * @param \Magento\Framework\View\Model\Layout\Update\Validator $validator
      * @param \Psr\Log\LoggerInterface $logger
-     * @param ReadFactory $readFactory ,
+     * @param ReadFactory $readFactory
      * @param \Magento\Framework\View\Design\ThemeInterface $theme Non-injectable theme instance
      * @param string $cacheSuffix
      * @param LayoutCacheKeyInterface $layoutCacheKey
+     * @param SerializerInterface|null $serializer
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
     public function __construct(
@@ -191,7 +199,8 @@ public function __construct(
         ReadFactory $readFactory,
         \Magento\Framework\View\Design\ThemeInterface $theme = null,
         $cacheSuffix = '',
-        LayoutCacheKeyInterface $layoutCacheKey = null
+        LayoutCacheKeyInterface $layoutCacheKey = null,
+        SerializerInterface $serializer = null
     ) {
         $this->theme = $theme ?: $design->getDesignTheme();
         $this->scope = $scopeResolver->getScope();
@@ -205,6 +214,7 @@ public function __construct(
         $this->cacheSuffix = $cacheSuffix;
         $this->layoutCacheKey = $layoutCacheKey
             ?: \Magento\Framework\App\ObjectManager::getInstance()->get(LayoutCacheKeyInterface::class);
+        $this->serializer = $serializer ?: ObjectManager::getInstance()->get(SerializerInterface::class);
     }
 
     /**
@@ -437,12 +447,12 @@ public function load($handles = [])
 
         $this->addHandle($handles);
 
-        $cacheId = $this->getCacheId();
-        $cacheIdPageLayout = $cacheId . '_' . self::PAGE_LAYOUT_CACHE_SUFFIX;
+        $cacheId = $this->getCacheId() . '_' . self::PAGE_LAYOUT_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;
             }
@@ -455,8 +465,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;
     }
 

lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php

@@ -28,11 +28,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
      */
@@ -53,10 +63,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();
@@ -70,10 +82,12 @@ 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,
                 'layoutCacheKey' => $this->layoutCacheKeyMock,
+                'serializer' => $this->serializer,
             ]
         );
     }
@@ -104,4 +118,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"     => "<body></body>"
+        ];
+
+        $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();
+    }
 }