mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 22:10:55 +01:00
When computing revision ownership, cache some intermediate results for performance
Summary: Ref T12319. With large datasets, the computation of which packages own paths in a revision is needlessly slow. Improve performance through caching: - Cache which paths belong to each repository. - Cache the split fragments of each path. - Cache the path fragment counts. - Micro-optimize accessing `$this->path`. Test Plan: - Used `bin/lipsum` to generate 4,000 packages with 150,000 paths. - Created a revision affecting 100 paths in `phabricator/` (these paths mostly overlap with `bin/lipsum` path rules, since Lipsum uses Phabricator-like rules to generate paths). - Before optimizations, this revision spent about 5.5 seconds computing paths. - After optimizations, it spends about 275ms. {F3423414} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12319 Differential Revision: https://secure.phabricator.com/D17424
This commit is contained in:
parent
b9568646ac
commit
3bea3fbb12
3 changed files with 41 additions and 13 deletions
|
@ -353,6 +353,9 @@ final class PhabricatorOwnersPackageQuery
|
|||
$packages = $this->controlResults;
|
||||
$weak_dominion = PhabricatorOwnersPackage::DOMINION_WEAK;
|
||||
|
||||
$path_fragments = PhabricatorOwnersPackage::splitPath($path);
|
||||
$fragment_count = count($path_fragments);
|
||||
|
||||
$matches = array();
|
||||
foreach ($packages as $package_id => $package) {
|
||||
$best_match = null;
|
||||
|
@ -365,13 +368,11 @@ final class PhabricatorOwnersPackageQuery
|
|||
continue;
|
||||
}
|
||||
|
||||
foreach ($package->getPaths() as $package_path) {
|
||||
if ($package_path->getRepositoryPHID() != $repository_phid) {
|
||||
// If this path is for some other repository, skip it.
|
||||
continue;
|
||||
}
|
||||
|
||||
$strength = $package_path->getPathMatchStrength($path);
|
||||
$repository_paths = $package->getPathsForRepository($repository_phid);
|
||||
foreach ($repository_paths as $package_path) {
|
||||
$strength = $package_path->getPathMatchStrength(
|
||||
$path_fragments,
|
||||
$fragment_count);
|
||||
if ($strength > $best_match) {
|
||||
$best_match = $strength;
|
||||
$include = !$package_path->getExcluded();
|
||||
|
|
|
@ -26,6 +26,7 @@ final class PhabricatorOwnersPackage
|
|||
private $paths = self::ATTACHABLE;
|
||||
private $owners = self::ATTACHABLE;
|
||||
private $customFields = self::ATTACHABLE;
|
||||
private $pathRepositoryMap = array();
|
||||
|
||||
const STATUS_ACTIVE = 'active';
|
||||
const STATUS_ARCHIVED = 'archived';
|
||||
|
@ -369,6 +370,10 @@ final class PhabricatorOwnersPackage
|
|||
public function attachPaths(array $paths) {
|
||||
assert_instances_of($paths, 'PhabricatorOwnersPath');
|
||||
$this->paths = $paths;
|
||||
|
||||
// Drop this cache if we're attaching new paths.
|
||||
$this->pathRepositoryMap = array();
|
||||
|
||||
return $this;
|
||||
}
|
||||
|
||||
|
@ -376,6 +381,23 @@ final class PhabricatorOwnersPackage
|
|||
return $this->assertAttached($this->paths);
|
||||
}
|
||||
|
||||
public function getPathsForRepository($repository_phid) {
|
||||
if (isset($this->pathRepositoryMap[$repository_phid])) {
|
||||
return $this->pathRepositoryMap[$repository_phid];
|
||||
}
|
||||
|
||||
$map = array();
|
||||
foreach ($this->getPaths() as $path) {
|
||||
if ($path->getRepositoryPHID() == $repository_phid) {
|
||||
$map[] = $path;
|
||||
}
|
||||
}
|
||||
|
||||
$this->pathRepositoryMap[$repository_phid] = $map;
|
||||
|
||||
return $this->pathRepositoryMap[$repository_phid];
|
||||
}
|
||||
|
||||
public function attachOwners(array $owners) {
|
||||
assert_instances_of($owners, 'PhabricatorOwnersOwner');
|
||||
$this->owners = $owners;
|
||||
|
|
|
@ -7,6 +7,9 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
|
|||
protected $path;
|
||||
protected $excluded;
|
||||
|
||||
private $fragments;
|
||||
private $fragmentCount;
|
||||
|
||||
protected function getConfiguration() {
|
||||
return array(
|
||||
self::CONFIG_TIMESTAMPS => false,
|
||||
|
@ -74,19 +77,21 @@ final class PhabricatorOwnersPath extends PhabricatorOwnersDAO {
|
|||
* Get the number of directory matches between this path specification and
|
||||
* some real path.
|
||||
*/
|
||||
public function getPathMatchStrength($path) {
|
||||
$this_path = $this->getPath();
|
||||
public function getPathMatchStrength($path_fragments, $path_count) {
|
||||
$this_path = $this->path;
|
||||
|
||||
if ($this_path === '/') {
|
||||
// The root path "/" just matches everything with strength 1.
|
||||
return 1;
|
||||
}
|
||||
|
||||
$self_fragments = PhabricatorOwnersPackage::splitPath($this_path);
|
||||
$path_fragments = PhabricatorOwnersPackage::splitPath($path);
|
||||
if ($this->fragments === null) {
|
||||
$this->fragments = PhabricatorOwnersPackage::splitPath($this_path);
|
||||
$this->fragmentCount = count($this->fragments);
|
||||
}
|
||||
|
||||
$self_count = count($self_fragments);
|
||||
$path_count = count($path_fragments);
|
||||
$self_fragments = $this->fragments;
|
||||
$self_count = $this->fragmentCount;
|
||||
if ($self_count > $path_count) {
|
||||
// If this path is longer (and therefore more specific) than the target
|
||||
// path, we don't match it at all.
|
||||
|
|
Loading…
Reference in a new issue