aaron(a)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
