r82468 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r82467‎ | r82468 | r82469 >
Date:17:07, 19 February 2011
Author:catrope
Status:ok
Tags:
Comment:
(bug 27302) Avoid unnecessary requests for user and site modules if the relevant wiki pages don't exist.

Done by adding isKnownEmpty() to ResourceLoaderModule and overriding it to check for page existence in ResourceLoaderWikiModule. Needed to rearrange some code in OutputPage::makeResourceLoaderLink() to have the emptiness check and dropping of modules work properly. Also factored the page_touched check in ResourceLoaderWikiModule::getModifiedTime() out to a separate method (getTitleMtimes()) and moved in-object caching there as well, so getModifiedTime() and isKnownEmpty() share code and caching for their timestamp/existence checks.

This does not account for the case where e.g. a user has user CSS but no user JS: I had implemented this by checking for $context->getOnly() in getTitleMtimes(), but then realized it's not safe to do this in a function called by getModifiedTime(): it causes the timestamp list in the startup module to only take scripts in account for wiki modules, because the startup module has &only=scripts set
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
@@ -2462,14 +2462,29 @@
24632463
24642464 $links = '';
24652465 foreach ( $groups as $group => $modules ) {
2466 - $query['modules'] = implode( '|', array_keys( $modules ) );
24672466 // Special handling for user-specific groups
24682467 if ( ( $group === 'user' || $group === 'private' ) && $wgUser->isLoggedIn() ) {
24692468 $query['user'] = $wgUser->getName();
24702469 }
 2470+
 2471+ // Create a fake request based on the one we are about to make so modules return
 2472+ // correct timestamp and emptiness data
 2473+ $context = new ResourceLoaderContext( $resourceLoader, new FauxRequest( $query ) );
 2474+ // Drop modules that know they're empty
 2475+ foreach ( $modules as $key => $module ) {
 2476+ if ( $module->isKnownEmpty( $context ) ) {
 2477+ unset( $modules[$key] );
 2478+ }
 2479+ }
 2480+ // If there are no modules left, skip this group
 2481+ if ( $modules === array() ) {
 2482+ continue;
 2483+ }
 2484+
 2485+ $query['modules'] = implode( '|', array_keys( $modules ) );
 2486+
24712487 // Support inlining of private modules if configured as such
24722488 if ( $group === 'private' && $wgResourceLoaderInlinePrivateModules ) {
2473 - $context = new ResourceLoaderContext( $resourceLoader, new FauxRequest( $query ) );
24742489 if ( $only == ResourceLoaderModule::TYPE_STYLES ) {
24752490 $links .= Html::inlineStyle(
24762491 $resourceLoader->makeModuleResponse( $context, $modules )
@@ -2487,9 +2502,6 @@
24882503 // on-wiki like site or user pages, or user preferences; we need to find the highest
24892504 // timestamp of these user-changable modules so we can ensure cache misses on change
24902505 if ( $group === 'user' || $group === 'site' ) {
2491 - // Create a fake request based on the one we are about to make so modules return
2492 - // correct times
2493 - $context = new ResourceLoaderContext( $resourceLoader, new FauxRequest( $query ) );
24942506 // Get the maximum timestamp
24952507 $timestamp = 1;
24962508 foreach ( $modules as $module ) {
Index: trunk/phase3/includes/resourceloader/ResourceLoaderModule.php
@@ -279,4 +279,17 @@
280280 // 0 would mean now
281281 return 1;
282282 }
 283+
 284+ /**
 285+ * Check whether this module is known to be empty. If a child class
 286+ * has an easy and cheap way to determine that this module is
 287+ * definitely going to be empty, it should override this method to
 288+ * return true in that case. Callers may optimize the request for this
 289+ * module away if this function returns true.
 290+ * @param $context ResourceLoaderContext: Context object
 291+ * @return Boolean
 292+ */
 293+ public function isKnownEmpty( ResourceLoaderContext $context ) {
 294+ return false;
 295+ }
283296 }
Index: trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -36,8 +36,8 @@
3737 # Origin is user-supplied code
3838 protected $origin = self::ORIGIN_USER_SITEWIDE;
3939
40 - // In-object cache for modified time
41 - protected $modifiedTime = array();
 40+ // In-object cache for title mtimes
 41+ protected $titleMtimes = array();
4242
4343 /* Abstract Protected Methods */
4444
@@ -120,32 +120,18 @@
121121 }
122122
123123 public function getModifiedTime( ResourceLoaderContext $context ) {
124 - $hash = $context->getHash();
125 - if ( isset( $this->modifiedTime[$hash] ) ) {
126 - return $this->modifiedTime[$hash];
127 - }
128 -
129 - $batch = new LinkBatch;
130 - foreach ( $this->getPages( $context ) as $titleText => $options ) {
131 - $batch->addObj( Title::newFromText( $titleText ) );
132 - }
133 -
134124 $modifiedTime = 1; // wfTimestamp() interprets 0 as "now"
135 - if ( !$batch->isEmpty() ) {
136 - $dbr = wfGetDB( DB_SLAVE );
137 - $latest = $dbr->selectField( 'page', 'MAX(page_touched)',
138 - $batch->constructSet( 'page', $dbr ),
139 - __METHOD__ );
140 -
141 - if ( $latest ) {
142 - $modifiedTime = wfTimestamp( TS_UNIX, $latest );
143 - }
 125+ $mtimes = $this->getTitleMtimes( $context );
 126+ if ( count( $mtimes ) ) {
 127+ $modifiedTime = max( $modifiedTime, max( $mtimes ) );
144128 }
145 -
146 - $this->modifiedTime[$hash] = $modifiedTime;
147129 return $modifiedTime;
148130 }
149 -
 131+
 132+ public function isKnownEmpty( ResourceLoaderContext $context ) {
 133+ return count( $this->getTitleMtimes( $context ) ) == 0;
 134+ }
 135+
150136 /**
151137 * @param $context ResourceLoaderContext
152138 * @return bool
@@ -155,4 +141,38 @@
156142
157143 return $wgContLang->getDir() !== $context->getDirection();
158144 }
 145+
 146+ /**
 147+ * Get the modification times of all titles that would be loaded for
 148+ * a given context.
 149+ * @param $context ResourceLoaderContext: Context object
 150+ * @return array( prefixed DB key => UNIX timestamp ), nonexistent titles are dropped
 151+ */
 152+ protected function getTitleMtimes( ResourceLoaderContext $context ) {
 153+ $hash = $context->getHash();
 154+ if ( isset( $this->titleMtimes[$hash] ) ) {
 155+ return $this->titleMtimes[$hash];
 156+ }
 157+
 158+ $this->titleMtimes[$hash] = array();
 159+ $batch = new LinkBatch;
 160+ foreach ( $this->getPages( $context ) as $titleText => $options ) {
 161+ $batch->addObj( Title::newFromText( $titleText ) );
 162+ }
 163+
 164+ if ( !$batch->isEmpty() ) {
 165+ $dbr = wfGetDB( DB_SLAVE );
 166+ $res = $dbr->select( 'page',
 167+ array( 'page_namespace', 'page_title', 'page_touched' ),
 168+ $batch->constructSet( 'page', $dbr ),
 169+ __METHOD__
 170+ );
 171+ foreach ( $res as $row ) {
 172+ $title = Title::makeTitle( $row->page_namespace, $row->page_title );
 173+ $this->titleMtimes[$hash][$title->getPrefixedDBkey()] =
 174+ wfTimestamp( TS_UNIX, $row->page_touched );
 175+ }
 176+ }
 177+ return $this->titleMtimes[$hash];
 178+ }
159179 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r846131.17wmf1: MFT r81692, r82468, r83814, r83885, r83891, r83897, r83902, r83903,...catrope17:42, 23 March 2011
r85211MFT: r82297, r82307, r82309, r82312, r82315, r82337, r82391, r82392, r82403, ...demon21:01, 2 April 2011
r85342MFT r82465, r82468, r82474 (second try), followup to r85211demon17:05, 4 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r82219(bug 27302) Don't append the current timestamp for user/site modules when no ...catrope07:19, 16 February 2011

Status & tagging log