From 7b50b2fbdc73e5bebc9b07bfe1175f347fe3060e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Jun 2012 15:21:48 -0700 Subject: [PATCH] Use subprocess output streaming to improve performance of Git commit discovery Summary: Improve performance of large discovery tasks in Git by using subprocess streaming, like we do for Mercurial. Basically, we save the cost of running many `git log` commands by running one big `git log` command but only parsing as much of it as we need to. This is pretty complicated, but we more or less need it for mercurial (which has ~100ms of 'hg' overhead instead of ~5ms of 'git' overhead) so we're already committed to most of the complexity costs. The git implementation is much simpler than the hg implementation because we don't need to handle all the weird parent rules (git gives us to them easily). Test Plan: Before, `discover --repair` on Phabricator took 35s: real 0m35.324s user 0m13.364s sys 0m21.088s Now 7s: real 0m7.236s user 0m2.436s sys 0m3.444s Note that most of the time is spent inserting rows after discover, the actual speedup of the git discovery part is much larger (subjectively, it runs in less than a second now, from ~28 seconds before). Also ran discover/pull on single new commits in normal cases to verify that nothing broke in the common case. Reviewers: jungejason, nh, vrana Reviewed By: vrana CC: aran Maniphest Tasks: T1401 Differential Revision: https://secure.phabricator.com/D2851 --- src/__phutil_library_map__.php | 1 + .../daemon/PhabricatorGitGraphStream.php | 96 +++++++++++++++++++ .../PhabricatorRepositoryPullLocalDaemon.php | 11 +-- 3 files changed, 101 insertions(+), 7 deletions(-) create mode 100644 src/applications/repository/daemon/PhabricatorGitGraphStream.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ea44bae3a9..43580a51f7 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -684,6 +684,7 @@ phutil_register_library_map(array( 'PhabricatorFlagQuery' => 'applications/flag/query/PhabricatorFlagQuery.php', 'PhabricatorFormExample' => 'applications/uiexample/examples/PhabricatorFormExample.php', 'PhabricatorGarbageCollectorDaemon' => 'infrastructure/daemon/PhabricatorGarbageCollectorDaemon.php', + 'PhabricatorGitGraphStream' => 'applications/repository/daemon/PhabricatorGitGraphStream.php', 'PhabricatorGoodForNothingWorker' => 'infrastructure/daemon/workers/worker/PhabricatorGoodForNothingWorker.php', 'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/PhabricatorHandleObjectSelectorDataView.php', 'PhabricatorHash' => 'infrastructure/util/PhabricatorHash.php', diff --git a/src/applications/repository/daemon/PhabricatorGitGraphStream.php b/src/applications/repository/daemon/PhabricatorGitGraphStream.php new file mode 100644 index 0000000000..62a2382098 --- /dev/null +++ b/src/applications/repository/daemon/PhabricatorGitGraphStream.php @@ -0,0 +1,96 @@ +repository = $repository; + + $future = $repository->getLocalCommandFuture( + "log --format=%s %s --", + '%H%x01%P%x01%ct', + $start_commit); + + $this->iterator = new LinesOfALargeExecFuture($future); + $this->iterator->setDelimiter("\n"); + $this->iterator->rewind(); + } + + public function getParents($commit) { + if (!isset($this->parents[$commit])) { + $this->parseUntil($commit); + } + return $this->parents[$commit]; + } + + public function getCommitDate($commit) { + if (!isset($this->dates[$commit])) { + $this->parseUntil($commit); + } + return $this->dates[$commit]; + } + + private function parseUntil($commit) { + if ($this->isParsed($commit)) { + return; + } + + $gitlog = $this->iterator; + + while ($gitlog->valid()) { + $line = $gitlog->current(); + $gitlog->next(); + + $line = trim($line); + if (!strlen($line)) { + break; + } + list($hash, $parents, $epoch) = explode("\1", $line); + + if ($parents) { + $parents = explode(' ', $parents); + } else { + // First commit. + $parents = array(); + } + + $this->dates[$hash] = $epoch; + $this->parents[$hash] = $parents; + + if ($this->isParsed($commit)) { + return; + } + } + + throw new Exception("No such commit '{$commit}' in repository!"); + } + + private function isParsed($commit) { + return isset($this->dates[$commit]); + } + +} diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index b887404df8..4eb9ccebeb 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -614,12 +614,11 @@ final class PhabricatorRepositoryPullLocalDaemon $seen_parent = array(); + $stream = new PhabricatorGitGraphStream($repository, $commit); + while (true) { $target = array_pop($discover); - list($parents) = $repository->execxLocalCommand( - 'log -n1 --pretty="%%P" %s', - $target); - $parents = array_filter(explode(' ', trim($parents))); + $parents = $stream->getParents($target); foreach ($parents as $parent) { if (isset($seen_parent[$parent])) { // We end up in a loop here somehow when we parse Arcanist if we @@ -656,9 +655,7 @@ final class PhabricatorRepositoryPullLocalDaemon while (true) { $target = array_pop($insert); - list($epoch) = $repository->execxLocalCommand( - 'log -n1 --pretty="%%ct" %s', - $target); + $epoch = $stream->getCommitDate($target); $epoch = trim($epoch); if ($branch !== null) {