r57980 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r57979‎ | r57980 | r57981 >
Date:17:35, 21 October 2009
Author:catrope
Status:ok (Comments)
Tags:
Comment:
* Export the return value of wfUrlProtocols() to JS for use in the EditToolbar extension
* Also cache this value (in a static var for now, maybe put it in memcached?): when parsing pages with many external links, we'd end up calling implode() once and preg_quote() 11 times for every link
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/Skin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -2236,6 +2236,12 @@
22372237 */
22382238 function wfUrlProtocols() {
22392239 global $wgUrlProtocols;
 2240+
 2241+ // This function is called a lot, cache its return value
 2242+ // TODO: Cache this in memcached instead?
 2243+ static $retval = null;
 2244+ if ( !is_null( $retval ) )
 2245+ return $retval;
22402246
22412247 // Support old-style $wgUrlProtocols strings, for backwards compatibility
22422248 // with LocalSettings files from 1.5
——@@ -2244,10 +2250,12 @@
22452251 foreach ($wgUrlProtocols as $protocol)
22462252 $protocols[] = preg_quote( $protocol, '/' );
22472253
2248 - return implode( '|', $protocols );
 2254+ $retval = implode( '|', $protocols );
22492255 } else {
2250 - return $wgUrlProtocols;
 2256+ $retval = $wgUrlProtocols;
22512257 }
 2258+
 2259+ return $retval;
22522260 }
22532261
22542262 /**
Index: trunk/phase3/includes/Skin.php
——@@ -384,6 +384,7 @@
385385 $vars = array(
386386 'skin' => $skinName,
387387 'stylepath' => $wgStylePath,
 388+ 'urlprotocols' => wfUrlProtocols(),
388389 'wgArticlePath' => $wgArticlePath,
389390 'wgScriptPath' => $wgScriptPath,
390391 'wgScriptExtension' => $wgScriptExtension,

Follow-up revisions

RevisionCommit summaryAuthorDate
r61489Fixme on r57980: Memcached would be bad here, rename variable with wg prefix....demon13:57, 25 January 2010

Comments

#Comment by Tim Starling (talk | contribs)   00:43, 17 December 2009

The explode only takes 8us for a typical protocol list, memcached::get profiles at 1ms average. So if you implemented that TODO you'd slow down first call performance by a factor of about 120.

The variable name is wrong, it should be wgUrlProtocols not urlprotocols.

#Comment by Catrope (talk | contribs)   10:14, 17 December 2009

Ha, I guess explode() is faster than I gave it credit for.

I chose a different variable name on purpose, because I'm outputting a processed version of $wgUrlProtocols (a regex) as opposed to the literal value.

#Comment by Tim Starling (talk | contribs)   01:56, 18 December 2009

It doesn't matter. The first two variables in that list lack the wg prefix only for legacy reasons. And several other globals are processed in similar ways, it's not implied that the JS global has precisely the same value as the PHP global.

Note that you are setting JavaScript global variables, those aren't members of some special object. Without the prefix, they might conflict with client-side libraries, extensions, user scripts, etc.

#Comment by Catrope (talk | contribs)   10:27, 18 December 2009

Good point. I'll rename var.

Status & tagging log