Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

RestrictionStore: Remove short-circuit mode when fetching cascading sources

Browse source 
Almost every call to isCascadeProtected() (which uses short-circuit
mode) is followed by a call to getCascadeProtectionSources() (which
doesn't), so this attempted optimization (skipping a loop that does
some very cheap operations) actually results in worse performance
in the common case (because the result of the database query can't
be cached in short-circuit mode, and we must query it again), and
it makes the code really annoying to read or modify.

Relevant code: https://codesearch.wmcloud.org/search/?q=getCascadeProtectionSources\(|isCascadeProtected\(&excludeFiles=RestrictionStore.php|HISTORY|tests%2F

Change-Id: Ib9eb6cab28492776d40a10cbfb28e9c1cec8c1d2
(cherry picked from commit f9180c4a36fb8874fc0211f05a1eebaceb67aa0c)
This commit is contained in:
Bartosz Dziewoński 2025-02-15 00:19:25 +01:00 committed by Reedy
parent 9c9440249f
commit 3aab5f1f3d
1 changed files with 10 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
includes/Permissions/RestrictionStore.php
View file

@ -54,7 +54,6 @@ class RestrictionStore {
* ?array `create_protection` => value for getCreateProtection
* bool `cascade` => cascade restrictions on this page to included templates and images?
* array[] `cascade_sources` => the results of getCascadeProtectionSources
* bool `has_cascading` => Are cascading restrictions in effect on this page?
* ]
*/
private $cache = [];
@ -266,7 +265,7 @@ class RestrictionStore {
public function isCascadeProtected( PageIdentity $page ): bool {
$page->assertWiki( PageIdentity::LOCAL );
return $this->getCascadeProtectionSourcesInternal( $page, true );
return $this->getCascadeProtectionSourcesInternal( $page )[0] !== [];
}
/**
@ -531,36 +530,32 @@ class RestrictionStore {
public function getCascadeProtectionSources( PageIdentity $page ): array {
$page->assertWiki( PageIdentity::LOCAL );
return $this->getCascadeProtectionSourcesInternal( $page, false );
return $this->getCascadeProtectionSourcesInternal( $page );
}
/**
* Cascading protection: Get the source of any cascading restrictions on this page.
*
* @param PageIdentity $page Must be local
* @param bool $shortCircuit If true, just return true or false instead of the actual lists.
* @return array|bool If $shortCircuit is true, return true if there is some cascading
* protection and false otherwise. Otherwise, same as getCascadeProtectionSources().
* @return array[] Same as getCascadeProtectionSources().
*/
private function getCascadeProtectionSourcesInternal(
PageIdentity $page, bool $shortCircuit = false
) {
PageIdentity $page
): array {
if ( !$page->canExist() ) {
return $shortCircuit ? false : [ [], [], [], [] ];
return [ [], [], [], [] ];
}
$cacheEntry = &$this->cache[CacheKeyHelper::getKeyForPage( $page )];
if ( !$shortCircuit && isset( $cacheEntry['cascade_sources'] ) ) {
if ( isset( $cacheEntry['cascade_sources'] ) ) {
return $cacheEntry['cascade_sources'];
} elseif ( $shortCircuit && isset( $cacheEntry['has_cascading'] ) ) {
return $cacheEntry['has_cascading'];
}
$dbr = $this->loadBalancer->getConnection( DB_REPLICA );
$baseQuery = $dbr->newSelectQueryBuilder()
->select( $shortCircuit ? [ 'pr_expiry' ] : [
->select( [
'pr_expiry',
'pr_page',
'page_namespace',
@ -569,12 +564,9 @@ class RestrictionStore {
'pr_level'
] )
->from( 'page_restrictions' )
->join( 'page', null, 'page_id=pr_page' )
->where( [ 'pr_cascade' => 1 ] );
if ( !$shortCircuit ) {
$baseQuery->join( 'page', null, 'page_id=pr_page' );
}
$imageQuery = clone $baseQuery;
$imageQuery->join( 'imagelinks', null, 'il_from=pr_page' )
->fields( [
@ -607,14 +599,8 @@ class RestrictionStore {
$pageRestrictions = [];
$now = wfTimestampNow();
foreach ( $res as $row ) {
$expiry = $dbr->decodeExpiry( $row->pr_expiry );
if ( $expiry > $now ) {
if ( $shortCircuit ) {
$cacheEntry['has_cascading'] = true;
return true;
}
if ( $row->type === 'il' ) {
$ilSources[$row->pr_page] = new PageIdentityValue( $row->pr_page,
$row->page_namespace, $row->page_title, PageIdentity::LOCAL );
@ -638,14 +624,9 @@ class RestrictionStore {
$sources = array_replace( $tlSources, $ilSources );
$cacheEntry['has_cascading'] = (bool)$sources;
$cacheEntry['cascade_sources'] = [ $sources, $pageRestrictions, $tlSources, $ilSources ];
if ( $shortCircuit ) {
return false;
}
return [ $sources, $pageRestrictions, $tlSources, $ilSources ];
return $cacheEntry['cascade_sources'];
}
/**