aaron@svn.wikimedia.org wrote:

Revision: 39675 Author: aaron Date: 2008-08-20 01:03:01 +0000 (Wed, 20 Aug 2008)

Log Message:

Remark static functions for performance. Same 15 test failures as before this change.

Modified Paths:

trunk/phase3/includes/parser/Parser.php

[...]

* @param string &$after set to everything after the ':'
* return string the position of the ':', or false if none found
*/

• function findColonNoLinks($str, &$before, &$after) {

• static function findColonNoLinks($str, &$before, &$after) {

There are several problems with this.

First, the performance gain. I measured a performance difference of only 150ns per call, on my laptop. This is extremely small. I could only reliably detect it with a tight unrolled loop of function calls. For comparison, this baseline loop took 120ns per iteration:

for ( $i = 0; $i < 10000000; $i++ ) { }

Suffice to say that anything at all that you do in a practical PHP program will swamp the effect of replacing object calls with static calls.

So it has no significant performance benefit, but it also makes maintaining the code difficult. If you later decide that it would be convenient for the functions in question to access elements of $this, a quite reasonable expectation given that the parser is an object for storing memoized results, then all calls will have to be changed to access an object. Any calls using the static syntax will become fatal errors -- including in unmaintained extensions that aren't in our repository.

Not only that, but static functions in classes such as Parser derail our attempts to implement a runtime-extensible object model. We now have the ability to plug in alternate parsers at runtime, such as Parser_OldPP and Parser_DiffTest. This is only possible when you never name the parser class in code external to that class. If an extension makes a call like this:

$pos = Parser::findColonNoLinks($s, $before, $after);

then they have broken the object model, made differential unit testing more difficult, and possibly introduced bugs. In theory, the correct call would be:

$class = get_class( $wgParser ); $pos = call_user_func_array( array( $class, 'findColonNoLinks' ), array( $s, &$before, &$after) );

But who's going to bother with that, right? This is simpler and faster:

$pos = $wgParser->findColonNoLinks($s, $before, $after);

The third problem is that PHP (before 5.3) doesn't resolve static calls to self:: using the object context, instead it uses the lexical context. If an extension creates a plug-in parser called ParserChild which extends Parser, then when Parser calls self::foo(), it will resolve to Parser::foo(), instead of ParserChild::foo(). This denies the child class the ability to override that functionality.

I think classes such as Parser should not use static functions at all, except for certain well-defined administrative roles, such as factory functions. This applies not only to the classes which already have dynamic names at runtime (Database, Parser, File, Article), but also to classes which would benefit from this kind of architecture in the future (User, OutputPage, SiteConfiguration).

-- Tim Starling