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

SiteConfiguration: Optimise getAll() and doReplace() loops

Browse source 
The runtime numbers in each bullet point are based on a local
benchmark that use a copy of WMF's InitialiseSettings.php file
to compute enwiki's $globals via SiteConfiguration::getAll().
Source code and results can be found at
<https://gist.github.com/Krinkle/2ef2fe72218b5f5cea91679df56a0b18>.

* The getSetting() method (called by getAll) was iterating over
  each find/replace pair and performing a separate call to str_replace()
  via doReplace().

  Instead, iterate the pairs only to create a replacements array
  (this array differs from the pairs in that it has a leading "$" in the
  search keys), and then call doReplace() once for each setting value,
  instead of multiple times.

  Also, switch from str_replace() to strtr() while at it (which promises
  to only search the string once, instead of repeatedly until it no
  longer finds any of the search patterns).

  Also also, generate the replacements array outside the getSetting()
  method and do this in the mergeParams() method instead, which is called
  before the getSetting-loop in getArray begins, thus further reducing
  identically repeated work.

  In my local benchmark for WMF/enwiki this reduced runtime from
  ~7.5-8.1ms to ~6.9-7.3ms.

* The getSetting() method was using a by-reference assignment
  for a local variable that would hold a read-only copy a class member
  `$thisSetting =& $this->settings[$settingName];`.

  This was presumably a PHP4 or PHP5 era optimisation to avoid
  an eager memory copy given the promise to only read the data.
  However, in PHP5/PHP7 this is redundant given that PHP
  does copy-on-write.

  Removing the "&" symbol here made a drastic perf improvement,
  reducing my WMF/enwiki benchmark runtime from ~6ms to ~1ms.

Bug: T169821
Change-Id: I0e8b395920c143f2b42f9d87c1afaffa2bfdaf2e
This commit is contained in:
Timo Tijhof 2020-03-02 17:47:15 +00:00 committed by Jforrester
parent 72c86bc25b
commit 268759c154
1 changed files with 19 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
includes/SiteConfiguration.php
View file

@ -203,7 +203,7 @@ class SiteConfiguration {
protected function getSetting( $settingName, $wiki, array $params ) {
$retval = null;
if ( array_key_exists( $settingName, $this->settings ) ) {
$thisSetting =& $this->settings[$settingName];
$thisSetting = $this->settings[$settingName];
do {
// Do individual wiki settings
if ( array_key_exists( $wiki, $thisSetting ) ) {
@ -261,34 +261,31 @@ class SiteConfiguration {
} while ( false );
}
if ( $retval !== null && count( $params['params'] ) ) {
foreach ( $params['params'] as $key => $value ) {
$retval = $this->doReplace( '$' . $key, $value, $retval );
}
if ( $retval !== null ) {
$retval = $this->doReplace( $retval, $params['replacements'] );
}
return $retval;
}
/**
* Type-safe string replace; won't do replacements on non-strings
* private?
*
* @param string $from
* @param string $to
* @param string|array $in
* @param string[] $replacements As provided by self::mergeParams()
* @return string|array
*/
function doReplace( $from, $to, $in ) {
private function doReplace( $in, array $replacements ) {
if ( is_string( $in ) ) {
return str_replace( $from, $to, $in );
$in = strtr( $in, $replacements );
} elseif ( is_array( $in ) ) {
foreach ( $in as $key => $val ) {
$in[$key] = $this->doReplace( $from, $to, $val );
if ( is_string( $val ) ) {
$in[$key] = strtr( $val, $replacements );
}
}
return $in;
} else {
return $in;
}
return $in;
}
/**
@ -478,6 +475,14 @@ class SiteConfiguration {
$ret['params']['site'] = $ret['suffix'];
}
// Optimisation for getAll() and extractAllGlobals():
// Precompute the replacements once when we process the params,
// instead separately for each of the thousands of variables.
$ret['replacements'] = [];
foreach ( $ret['params'] as $key => $value ) {
$ret['replacements'][ '$' . $key ] = $value;
}
return $ret;
}