From 2caf69797ccde49323e3cedb79f70268b8d76fdd Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Thu, 23 Mar 2023 10:48:49 -0400 Subject: [PATCH] ParserOutputAccess: Fork primary and secondary caches for parsoid Uses flag to detect which cache instance to use based on ParserOptions and sets the primary and secondary caches accordingly. This ensures that the ParserCacheMetadata cache used by the ParserCache is also appropriately forked for Parsoid, as Parsoid may consult different options in the ParserCache than core does. A follow up patch will attempt to refactor this to be less parsoid-specific. Bug: T327769 Bug: T330677 Co-authored-by: Alangi Derick Change-Id: Id580b97ad9a0b90bbe56d4de3c2f999274fe329b --- includes/ServiceWiring.php | 3 +- includes/page/ParserOutputAccess.php | 72 +++++++---- includes/parser/ParserCacheFactory.php | 3 + .../includes/page/ParserOutputAccessTest.php | 122 +++++++++++++++++- 4 files changed, 173 insertions(+), 27 deletions(-) diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 46053d57845..d23343d8a57 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -1355,8 +1355,7 @@ return [ 'ParserOutputAccess' => static function ( MediaWikiServices $services ): ParserOutputAccess { return new ParserOutputAccess( - $services->getParserCache(), - $services->getParserCacheFactory()->getRevisionOutputCache( 'rcache' ), + $services->getParserCacheFactory(), $services->getRevisionLookup(), $services->getRevisionRenderer(), $services->getStatsdDataFactory(), diff --git a/includes/page/ParserOutputAccess.php b/includes/page/ParserOutputAccess.php index 022d6573ae6..ce861c09af0 100644 --- a/includes/page/ParserOutputAccess.php +++ b/includes/page/ParserOutputAccess.php @@ -22,6 +22,7 @@ namespace MediaWiki\Page; use IBufferingStatsdDataFactory; use InvalidArgumentException; use MediaWiki\Logger\Spi as LoggerSpi; +use MediaWiki\Parser\ParserCacheFactory; use MediaWiki\Parser\RevisionOutputCache; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; @@ -84,13 +85,7 @@ class ParserOutputAccess { /** @var string Use secondary cache */ private const CACHE_SECONDARY = 'secondary'; - /** @var ParserCache */ - private $primaryCache; - - /** - * @var RevisionOutputCache - */ - private $secondaryCache; + private ParserCacheFactory $parserCacheFactory; /** * In cases that an extension tries to get the same ParserOutput of @@ -121,8 +116,7 @@ class ParserOutputAccess { private $titleFormatter; /** - * @param ParserCache $primaryCache - * @param RevisionOutputCache $secondaryCache + * @param ParserCacheFactory $parserCacheFactory * @param RevisionLookup $revisionLookup * @param RevisionRenderer $revisionRenderer * @param IBufferingStatsdDataFactory $statsDataFactory @@ -132,8 +126,7 @@ class ParserOutputAccess { * @param TitleFormatter $titleFormatter */ public function __construct( - ParserCache $primaryCache, - RevisionOutputCache $secondaryCache, + ParserCacheFactory $parserCacheFactory, RevisionLookup $revisionLookup, RevisionRenderer $revisionRenderer, IBufferingStatsdDataFactory $statsDataFactory, @@ -142,8 +135,7 @@ class ParserOutputAccess { WikiPageFactory $wikiPageFactory, TitleFormatter $titleFormatter ) { - $this->primaryCache = $primaryCache; - $this->secondaryCache = $secondaryCache; + $this->parserCacheFactory = $parserCacheFactory; $this->revisionLookup = $revisionLookup; $this->revisionRenderer = $revisionRenderer; $this->statsDataFactory = $statsDataFactory; @@ -210,15 +202,17 @@ class ParserOutputAccess { ): ?ParserOutput { $isOld = $revision && $revision->getId() !== $page->getLatest(); $useCache = $this->shouldUseCache( $page, $revision ); - $classCacheKey = $this->primaryCache->makeParserOutputKey( $page, $parserOptions ); + $primaryCache = $this->getPrimaryCache( $parserOptions ); + $classCacheKey = $primaryCache->makeParserOutputKey( $page, $parserOptions ); if ( $useCache === self::CACHE_PRIMARY ) { if ( isset( $this->localCache[$classCacheKey] ) && !$isOld ) { return $this->localCache[$classCacheKey]; } - $output = $this->primaryCache->get( $page, $parserOptions ); + $output = $primaryCache->get( $page, $parserOptions ); } elseif ( $useCache === self::CACHE_SECONDARY && $revision ) { - $output = $this->secondaryCache->get( $revision, $parserOptions ); + $secondaryCache = $this->getSecondaryCache( $parserOptions ); + $output = $secondaryCache->get( $revision, $parserOptions ); } else { $output = null; } @@ -301,7 +295,8 @@ class ParserOutputAccess { Assert::postcondition( $output || !$status->isOK(), 'Worker returned invalid status' ); if ( $output && !$isOld ) { - $classCacheKey = $this->primaryCache->makeParserOutputKey( $page, $parserOptions ); + $primaryCache = $this->getPrimaryCache( $parserOptions ); + $classCacheKey = $primaryCache->makeParserOutputKey( $page, $parserOptions ); $this->localCache[$classCacheKey] = $output; } @@ -375,12 +370,13 @@ class ParserOutputAccess { int $options ): PoolCounterWork { $useCache = $this->shouldUseCache( $page, $revision ); + $primaryCache = $this->getPrimaryCache( $parserOptions ); switch ( $useCache ) { case self::CACHE_PRIMARY: $this->statsDataFactory->increment( 'ParserOutputAccess.PoolWork.Current' ); - $parserCacheMetadata = $this->primaryCache->getMetadata( $page ); - $cacheKey = $this->primaryCache->makeParserOutputKey( $page, $parserOptions, + $parserCacheMetadata = $primaryCache->getMetadata( $page ); + $cacheKey = $primaryCache->makeParserOutputKey( $page, $parserOptions, $parserCacheMetadata ? $parserCacheMetadata->getUsedOptions() : null ); @@ -392,7 +388,7 @@ class ParserOutputAccess { $revision, $parserOptions, $this->revisionRenderer, - $this->primaryCache, + $primaryCache, $this->lbFactory, $this->loggerSpi, $this->wikiPageFactory, @@ -401,10 +397,11 @@ class ParserOutputAccess { case self::CACHE_SECONDARY: $this->statsDataFactory->increment( 'ParserOutputAccess.PoolWork.Old' ); - $workKey = $this->secondaryCache->makeParserOutputKey( $revision, $parserOptions ); + $secondaryCache = $this->getSecondaryCache( $parserOptions ); + $workKey = $secondaryCache->makeParserOutputKey( $revision, $parserOptions ); return new PoolWorkArticleViewOld( $workKey, - $this->secondaryCache, + $secondaryCache, $revision, $parserOptions, $this->revisionRenderer, @@ -413,7 +410,8 @@ class ParserOutputAccess { default: $this->statsDataFactory->increment( 'ParserOutputAccess.PoolWork.Uncached' ); - $workKey = $this->secondaryCache->makeParserOutputKeyOptionalRevId( $revision, $parserOptions ); + $secondaryCache = $this->getSecondaryCache( $parserOptions ); + $workKey = $secondaryCache->makeParserOutputKeyOptionalRevId( $revision, $parserOptions ); return new PoolWorkArticleView( $workKey, $revision, @@ -426,4 +424,32 @@ class ParserOutputAccess { // unreachable } + private function getPrimaryCache( ParserOptions $pOpts ): ParserCache { + if ( $pOpts->getUseParsoid() ) { + // T331148: This is different from + // ParsoidOutputAccess::PARSOID_PARSER_CACHE_NAME; will be + // renamed once the contents cached on the read-views and + // the REST path are identical. + return $this->parserCacheFactory->getParserCache( + 'parsoid-' . ParserCacheFactory::DEFAULT_NAME + ); + } + + return $this->parserCacheFactory->getParserCache( + ParserCacheFactory::DEFAULT_NAME + ); + } + + private function getSecondaryCache( ParserOptions $pOpts ): RevisionOutputCache { + if ( $pOpts->getUseParsoid() ) { + return $this->parserCacheFactory->getRevisionOutputCache( + 'parsoid-' . ParserCacheFactory::DEFAULT_RCACHE_NAME + ); + } + + return $this->parserCacheFactory->getRevisionOutputCache( + ParserCacheFactory::DEFAULT_RCACHE_NAME + ); + } + } diff --git a/includes/parser/ParserCacheFactory.php b/includes/parser/ParserCacheFactory.php index 3f79d8d0609..9249a812234 100644 --- a/includes/parser/ParserCacheFactory.php +++ b/includes/parser/ParserCacheFactory.php @@ -43,6 +43,9 @@ class ParserCacheFactory { /** @var string name of ParserCache for the default parser */ public const DEFAULT_NAME = 'pcache'; + /** @var string name of RevisionOutputCache for the default parser */ + public const DEFAULT_RCACHE_NAME = 'rcache'; + /** @var BagOStuff */ private $parserCacheBackend; diff --git a/tests/phpunit/includes/page/ParserOutputAccessTest.php b/tests/phpunit/includes/page/ParserOutputAccessTest.php index f756cbb8bd3..72c73853061 100644 --- a/tests/phpunit/includes/page/ParserOutputAccessTest.php +++ b/tests/phpunit/includes/page/ParserOutputAccessTest.php @@ -3,6 +3,7 @@ use MediaWiki\Json\JsonCodec; use MediaWiki\Logger\Spi as LoggerSpi; use MediaWiki\MainConfigNames; use MediaWiki\Page\ParserOutputAccess; +use MediaWiki\Parser\ParserCacheFactory; use MediaWiki\Parser\RevisionOutputCache; use MediaWiki\PoolCounter\PoolCounterFactory; use MediaWiki\Revision\MutableRevisionRecord; @@ -130,6 +131,26 @@ class ParserOutputAccessTest extends MediaWikiIntegrationTestCase { $revisionOutputCache = $this->getRevisionOutputCache( new HashBagOStuff() ); } + $parserCacheFactory = $this->createMock( ParserCacheFactory::class ); + $parserCacheFactory->method( 'getParserCache' )->willReturn( $parserCache ); + $parserCacheFactory->method( 'getRevisionOutputCache' )->willReturn( $revisionOutputCache ); + return $this->getParserOutputAccessWithCacheFactory( + $parserCacheFactory, + $maxRenderCalls + ); + } + + /** + * @param ParserCacheFactory $parserCacheFactory + * @param int|bool $maxRenderCalls + * + * @return ParserOutputAccess + * @throws Exception + */ + private function getParserOutputAccessWithCacheFactory( + $parserCacheFactory, + $maxRenderCalls = false + ) { $revRenderer = $this->getServiceContainer()->getRevisionRenderer(); if ( $maxRenderCalls ) { @@ -143,8 +164,7 @@ class ParserOutputAccessTest extends MediaWikiIntegrationTestCase { } return new ParserOutputAccess( - $parserCache, - $revisionOutputCache, + $parserCacheFactory, $this->getServiceContainer()->getRevisionLookup(), $revRenderer, new NullStatsdDataFactory(), @@ -735,4 +755,102 @@ class ParserOutputAccessTest extends MediaWikiIntegrationTestCase { $this->assertContainsHtml( 'World', $result ); } + public function testParsoidCacheSplit() { + $parserCacheFactory = $this->createMock( ParserCacheFactory::class ); + $revisionOutputCache = $this->getRevisionOutputCache( new HashBagOStuff() ); + $caches = [ + $this->getParserCache( new HashBagOStuff() ), + $this->getParserCache( new HashBagOStuff() ), + ]; + $calls = []; + $parserCacheFactory + ->method( 'getParserCache' ) + ->willReturnCallback( static function ( $cacheName ) use ( &$calls, $caches ) { + static $cacheList = []; + $calls[] = $cacheName; + $which = array_search( $cacheName, $cacheList ); + if ( $which === false ) { + $which = count( $cacheList ); + $cacheList[] = $cacheName; + } + return $caches[$which]; + } ); + $parserCacheFactory + ->method( 'getRevisionOutputCache' ) + ->willReturn( $revisionOutputCache ); + + $access = $this->getParserOutputAccessWithCacheFactory( $parserCacheFactory ); + $parserOptions0 = $this->getParserOptions(); + $page = $this->getNonexistingTestPage( __METHOD__ ); + $output = $access->getCachedParserOutput( $page, $parserOptions0 ); + $this->assertNull( $output ); + // $calls[0] will remember what cache name we used. + $this->assertCount( 1, $calls ); + + $parserOptions1 = $this->getParserOptions(); + $parserOptions1->setUseParsoid(); + $output = $access->getCachedParserOutput( $page, $parserOptions1 ); + $this->assertNull( $output ); + $this->assertCount( 2, $calls ); + // Check that we used a different cache name this time. + $this->assertNotEquals( $calls[1], $calls[0], "Should use different caches" ); + + // Try this again, with actual content. + $calls = []; + $this->editPage( $page, "__NOTOC__" ); + $status0 = $access->getParserOutput( $page, $parserOptions0 ); + $this->assertContainsHtml( '
', $status0 ); + $status1 = $access->getParserOutput( $page, $parserOptions1 ); + $this->assertContainsHtml( 'assertNotSameHtml( $status0, $status1 ); + } + + public function testParsoidRevisionCacheSplit() { + $parserCacheFactory = $this->createMock( ParserCacheFactory::class ); + $parserCache = $this->getParserCache( new HashBagOStuff() ); + $caches = [ + $this->getRevisionOutputCache( new HashBagOStuff() ), + $this->getRevisionOutputCache( new HashBagOStuff() ), + ]; + $calls = []; + $parserCacheFactory + ->method( 'getParserCache' ) + ->willReturn( $parserCache ); + $parserCacheFactory + ->method( 'getRevisionOutputCache' ) + ->willReturnCallback( static function ( $cacheName ) use ( &$calls, $caches ) { + static $cacheList = []; + $calls[] = $cacheName; + $which = array_search( $cacheName, $cacheList ); + if ( $which === false ) { + $which = count( $cacheList ); + $cacheList[] = $cacheName; + } + return $caches[$which]; + } ); + + $access = $this->getParserOutputAccessWithCacheFactory( $parserCacheFactory ); + $page = $this->getNonexistingTestPage( __METHOD__ ); + $firstRev = $this->editPage( $page, 'First __NOTOC__' )->getNewRevision(); + $secondRev = $this->editPage( $page, 'Second __NOTOC__' )->getNewRevision(); + + $parserOptions0 = $this->getParserOptions(); + $status = $access->getParserOutput( $page, $parserOptions0, $firstRev ); + $this->assertContainsHtml( 'First', $status ); + // Check that we used the "not parsoid" revision cache + $this->assertTrue( count( $calls ) > 0 ); + $notParsoid = $calls[0]; + $this->assertEquals( array_fill( 0, count( $calls ), $notParsoid ), $calls ); + + $calls = []; + $parserOptions1 = $this->getParserOptions(); + $parserOptions1->setUseParsoid(); + $status = $access->getParserOutput( $page, $parserOptions1, $firstRev ); + $this->assertContainsHtml( 'First', $status ); + $this->assertContainsHtml( 'assertTrue( count( $calls ) > 0 ); + $parsoid = $calls[0]; + $this->assertNotEquals( $notParsoid, $parsoid, "Should use different caches" ); + $this->assertEquals( array_fill( 0, count( $calls ), $parsoid ), $calls ); + } }