1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-25 15:00:58 +01:00

Improve LiskDAO::__call() performance

Summary:
This is kind of expensive and can be significant on, e.g., the
Maniphest task list view. Do a little more caching and some clever nonsense to
improve performance.

Test Plan:
Local cost on Maniphest "all tasks" view for this method dropped from
##82,856us## to ##24,607us## on 9,061 calls.

I wrote some unit test / microbenchmark things:

  public function testGetIDCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->getID();
    }
    $this->assertEqual(1, 1);
  }

  public function testGetCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->getUsername();
    }
    $this->assertEqual(1, 1);
  }

  public function testSetCost() {
    $u = new PhabricatorUser();
    $n = 100000;
    while ($n--) {
      $u->setID(1);
    }
    $this->assertEqual(1, 1);
  }

Before:

   PASS  598ms   testSetCost
   PASS  584ms   testGetCost
   PASS  272ms   testGetIDCost

After:

   PASS  170ms   testSetCost
   PASS  207ms   testGetCost
   PASS   29ms   testGetIDCost

Also, ran unit tests.

Reviewers: nh, btrahan, jungejason

Reviewed By: nh

CC: aran, epriestley, nh

Differential Revision: https://secure.phabricator.com/D1291
This commit is contained in:
epriestley 2011-12-29 14:23:54 -08:00
parent 8f289e6687
commit 7831b92427
2 changed files with 103 additions and 23 deletions

View file

@ -616,7 +616,10 @@ abstract class LiskDAO {
* @task info * @task info
*/ */
public function getID() { public function getID() {
static $id_key = null;
if ($id_key === null) {
$id_key = $this->getIDKeyForUse(); $id_key = $this->getIDKeyForUse();
}
return $this->$id_key; return $this->$id_key;
} }
@ -1258,42 +1261,66 @@ abstract class LiskDAO {
* @task util * @task util
*/ */
public function __call($method, $args) { public function __call($method, $args) {
if (!strncmp($method, 'get', 3)) {
// NOTE: This method is very performance-sensitive (many thousands of calls
// per page on some pages), and thus has some silliness in the name of
// optimizations.
static $dispatch_map = array();
static $partial = null;
if ($partial === null) {
$partial = $this->getConfigOption(self::CONFIG_PARTIAL_OBJECTS);
}
if ($method[0] === 'g') {
if (isset($dispatch_map[$method])) {
$property = $dispatch_map[$method];
} else {
if (substr($method, 0, 3) !== 'get') {
throw new Exception("Unable to resolve method '{$method}'!");
}
$property = substr($method, 3); $property = substr($method, 3);
if (!($property = $this->checkProperty($property))) { if (!($property = $this->checkProperty($property))) {
throw new Exception("Bad getter call: {$method}"); throw new Exception("Bad getter call: {$method}");
} }
if (count($args) !== 0) { $dispatch_map[$method] = $property;
throw new Exception("Getter call should have zero args: {$method}");
} }
if ($this->getConfigOption(self::CONFIG_PARTIAL_OBJECTS) &&
isset($this->__missingFields[$property])) { if ($partial && isset($this->__missingFields[$property])) {
throw new Exception("Cannot get field that wasn't loaded: {$property}"); throw new Exception("Cannot get field that wasn't loaded: {$property}");
} }
return $this->readField($property); return $this->readField($property);
} }
if (!strncmp($method, 'set', 3)) { if ($method[0] === 's') {
if (isset($dispatch_map[$method])) {
$property = $dispatch_map[$method];
} else {
if (substr($method, 0, 3) !== 'set') {
throw new Exception("Unable to resolve method '{$method}'!");
}
$property = substr($method, 3); $property = substr($method, 3);
$property = $this->checkProperty($property); $property = $this->checkProperty($property);
if (!$property) { if (!$property) {
throw new Exception("Bad setter call: {$method}"); throw new Exception("Bad setter call: {$method}");
} }
if (count($args) !== 1) {
throw new Exception("Setter should have exactly one arg: {$method}");
}
if ($property == 'ID') { if ($property == 'ID') {
$property = $this->getIDKeyForUse(); $property = $this->getIDKeyForUse();
} }
if ($this->getConfigOption(self::CONFIG_PARTIAL_OBJECTS)) { $dispatch_map[$method] = $property;
}
if ($partial) {
// Accept writes to fields that weren't initially loaded // Accept writes to fields that weren't initially loaded
unset($this->__missingFields[$property]); unset($this->__missingFields[$property]);
$this->__dirtyFields[$property] = true; $this->__dirtyFields[$property] = true;
} }
$this->writeField($property, $args[0]); $this->writeField($property, $args[0]);
return $this; return $this;
} }
throw new Exception("Unable to resolve method: {$method}."); throw new Exception("Unable to resolve method '{$method}'.");
} }
} }

View file

@ -53,4 +53,57 @@ class LiskIsolationTestCase extends PhabricatorTestCase {
} }
public function testMagicMethods() {
$dao = new LiskIsolationTestDAO();
$this->assertEqual(
null,
$dao->getName(),
'getName() on empty object');
$this->assertEqual(
$dao,
$dao->setName('x'),
'setName() returns $this');
$this->assertEqual(
'y',
$dao->setName('y')->getName(),
'setName() has an effect');
$ex = null;
try {
$dao->gxxName();
} catch (Exception $thrown) {
$ex = $thrown;
}
$this->assertEqual(
true,
(bool)$ex,
'Typoing "get" should throw.');
$ex = null;
try {
$dao->sxxName('z');
} catch (Exception $thrown) {
$ex = $thrown;
}
$this->assertEqual(
true,
(bool)$ex,
'Typoing "set" should throw.');
$ex = null;
try {
$dao->madeUpMethod();
} catch (Exception $thrown) {
$ex = $thrown;
}
$this->assertEqual(
true,
(bool)$ex,
'Made up method should throw.');
}
} }