From 0aa913805d14cd069d868dcc93a9fe12b781e792 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 25 May 2014 09:28:11 -0700 Subject: [PATCH] Add an alternate "modern" hunk datastore Summary: Ref T4045. Ref T5179. Hunk storage has two major issues: - It's utf8, but actual diffs are binary. - It's huge and can't be compressed or archived. This introduces a second datastore which solves these problems: by recording hunk encoding, supporting compression, and supporting alternate storage. There's no actual compression or storage support yet, but there's space in the table for them. Since nothing actually uses hunk IDs, it's fine to have these tables exist at the same time and use the same IDs. We can migrate data between the tables gradually without requiring downtime or disrupting installs. Test Plan: - There are no writes to the new table yet. - The only effect this has is making us issue one extra query when looking for hunks. - Observed the query issue, but everything else continue working fine. - Created a new diff. - Ran unit tests. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4045, T5179 Differential Revision: https://secure.phabricator.com/D9290 --- .../sql/autopatches/20140525.hunkmodern.sql | 18 +++++ src/__phutil_library_map__.php | 4 + .../DifferentialChangesetParserTestCase.php | 4 +- .../DifferentialHunkParserTestCase.php | 2 +- .../query/DifferentialHunkQuery.php | 26 ++++++- .../storage/DifferentialChangeset.php | 2 +- .../differential/storage/DifferentialDiff.php | 2 +- .../differential/storage/DifferentialHunk.php | 3 +- .../storage/DifferentialHunkLegacy.php | 11 +++ .../storage/DifferentialHunkModern.php | 73 +++++++++++++++++++ .../__tests__/DifferentialHunkTestCase.php | 4 +- ...torRepositoryCommitMessageParserWorker.php | 4 +- 12 files changed, 139 insertions(+), 14 deletions(-) create mode 100644 resources/sql/autopatches/20140525.hunkmodern.sql create mode 100644 src/applications/differential/storage/DifferentialHunkLegacy.php create mode 100644 src/applications/differential/storage/DifferentialHunkModern.php diff --git a/resources/sql/autopatches/20140525.hunkmodern.sql b/resources/sql/autopatches/20140525.hunkmodern.sql new file mode 100644 index 0000000000..dd865e4d4d --- /dev/null +++ b/resources/sql/autopatches/20140525.hunkmodern.sql @@ -0,0 +1,18 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_hunk_modern ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + changesetID INT UNSIGNED NOT NULL, + oldOffset INT UNSIGNED NOT NULL, + oldLen INT UNSIGNED NOT NULL, + newOffset INT UNSIGNED NOT NULL, + newLen INT UNSIGNED NOT NULL, + dataType CHAR(4) NOT NULL COLLATE latin1_bin, + dataEncoding VARCHAR(16) COLLATE latin1_bin, + dataFormat CHAR(4) NOT NULL COLLATE latin1_bin, + data LONGBLOB NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + + KEY `key_changeset` (changesetID), + KEY `key_created` (dateCreated) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d1a08a7a98..f39e8dfb75 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -385,6 +385,8 @@ phutil_register_library_map(array( 'DifferentialHostField' => 'applications/differential/customfield/DifferentialHostField.php', 'DifferentialHovercardEventListener' => 'applications/differential/event/DifferentialHovercardEventListener.php', 'DifferentialHunk' => 'applications/differential/storage/DifferentialHunk.php', + 'DifferentialHunkLegacy' => 'applications/differential/storage/DifferentialHunkLegacy.php', + 'DifferentialHunkModern' => 'applications/differential/storage/DifferentialHunkModern.php', 'DifferentialHunkParser' => 'applications/differential/parser/DifferentialHunkParser.php', 'DifferentialHunkParserTestCase' => 'applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php', 'DifferentialHunkQuery' => 'applications/differential/query/DifferentialHunkQuery.php', @@ -3069,6 +3071,8 @@ phutil_register_library_map(array( 0 => 'DifferentialDAO', 1 => 'PhabricatorPolicyInterface', ), + 'DifferentialHunkLegacy' => 'DifferentialHunk', + 'DifferentialHunkModern' => 'DifferentialHunk', 'DifferentialHunkParserTestCase' => 'PhabricatorTestCase', 'DifferentialHunkQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialHunkTestCase' => 'ArcanistPhutilTestCase', diff --git a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php index d8d10169b5..d1f8876103 100644 --- a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php @@ -3,7 +3,7 @@ final class DifferentialChangesetParserTestCase extends PhabricatorTestCase { public function testDiffChangesets() { - $hunk = new DifferentialHunk(); + $hunk = new DifferentialHunkLegacy(); $hunk->setChanges("+a\n b\n-c"); $hunk->setNewOffset(1); $hunk->setNewLen(2); @@ -20,7 +20,7 @@ final class DifferentialChangesetParserTestCase extends PhabricatorTestCase { ); foreach ($tests as $changes => $expected) { - $hunk = new DifferentialHunk(); + $hunk = new DifferentialHunkLegacy(); $hunk->setChanges($changes); $hunk->setNewOffset(11); $hunk->setNewLen(3); diff --git a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php index 664a77f3ac..971b356775 100644 --- a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php +++ b/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php @@ -14,7 +14,7 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase { $new_len, $changes) { - $hunk = id(new DifferentialHunk()) + $hunk = id(new DifferentialHunkLegacy()) ->setOldOffset($old_offset) ->setOldLen($old_len) ->setNewOffset($new_offset) diff --git a/src/applications/differential/query/DifferentialHunkQuery.php b/src/applications/differential/query/DifferentialHunkQuery.php index 1e632b22fc..7a2d2b4dad 100644 --- a/src/applications/differential/query/DifferentialHunkQuery.php +++ b/src/applications/differential/query/DifferentialHunkQuery.php @@ -31,18 +31,38 @@ final class DifferentialHunkQuery } public function loadPage() { - $table = new DifferentialHunk(); + $all_results = array(); + + // Load modern hunks. + $table = new DifferentialHunkModern(); + $conn_r = $table->establishConnection('r'); + + $modern_data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + $modern_results = $table->loadAllFromArray($modern_data); + + + // Now, load legacy hunks. + $table = new DifferentialHunkLegacy(); $conn_r = $table->establishConnection('r'); - $data = queryfx_all( + $legacy_data = queryfx_all( $conn_r, 'SELECT * FROM %T %Q %Q %Q', $table->getTableName(), $this->buildWhereClause($conn_r), $this->buildOrderClause($conn_r), $this->buildLimitClause($conn_r)); + $legacy_results = $table->loadAllFromArray($legacy_data); - return $table->loadAllFromArray($data); + // Strip all the IDs off since they're not unique and nothing should be + // using them. + return array_values(array_merge($legacy_results, $modern_results)); } public function willFilterPage(array $hunks) { diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 9c2f017873..cffe77d83e 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -76,7 +76,7 @@ final class DifferentialChangeset extends DifferentialDAO public function delete() { $this->openTransaction(); - $hunks = id(new DifferentialHunk())->loadAllWhere( + $hunks = id(new DifferentialHunkLegacy())->loadAllWhere( 'changesetID = %d', $this->getID()); foreach ($hunks as $hunk) { diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 237fab7e59..7fe40ff04e 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -132,7 +132,7 @@ final class DifferentialDiff $hunks = $change->getHunks(); if ($hunks) { foreach ($hunks as $hunk) { - $dhunk = new DifferentialHunk(); + $dhunk = new DifferentialHunkLegacy(); $dhunk->setOldOffset($hunk->getOldOffset()); $dhunk->setOldLen($hunk->getOldLength()); $dhunk->setNewOffset($hunk->getNewOffset()); diff --git a/src/applications/differential/storage/DifferentialHunk.php b/src/applications/differential/storage/DifferentialHunk.php index 635bd1e413..39c5b78221 100644 --- a/src/applications/differential/storage/DifferentialHunk.php +++ b/src/applications/differential/storage/DifferentialHunk.php @@ -1,10 +1,9 @@ array( + 'data' => true, + ), + ) + parent::getConfiguration(); + } + + public function setChanges($text) { + $this->dataEncoding = $this->detectEncodingForStorage($text); + $this->dataType = self::DATATYPE_TEXT; + $this->dataFormat = self::DATAFORMAT_RAW; + $this->data = $text; + + return $this; + } + + public function getChanges() { + return $this->getUTF8StringFromStorage( + $this->getRawData(), + $this->getDataEncoding()); + } + + private function getRawData() { + $type = $this->getDataType(); + $data = $this->getData(); + + switch ($type) { + case self::DATATYPE_TEXT: + // In this storage type, the changes are stored on the object. + $data = $data; + break; + case self::DATATYPE_FILE: + default: + throw new Exception( + pht('Hunk has unsupported data type "%s"!', $type)); + } + + $format = $this->getDataFormat(); + switch ($format) { + case self::DATAFORMAT_RAW: + // In this format, the changes are stored as-is. + $data = $data; + break; + case self::DATAFORMAT_DEFLATE: + default: + throw new Exception( + pht('Hunk has unsupported data encoding "%s"!', $type)); + } + + return $data; + } + +} diff --git a/src/applications/differential/storage/__tests__/DifferentialHunkTestCase.php b/src/applications/differential/storage/__tests__/DifferentialHunkTestCase.php index 1e1e7e927d..6e4d499eb8 100644 --- a/src/applications/differential/storage/__tests__/DifferentialHunkTestCase.php +++ b/src/applications/differential/storage/__tests__/DifferentialHunkTestCase.php @@ -5,7 +5,7 @@ final class DifferentialHunkTestCase extends ArcanistPhutilTestCase { public function testMakeChanges() { $root = dirname(__FILE__).'/hunk/'; - $hunk = new DifferentialHunk(); + $hunk = new DifferentialHunkLegacy(); $hunk->setChanges(Filesystem::readFile($root.'basic.diff')); $hunk->setOldOffset(1); $hunk->setNewOffset(11); @@ -23,7 +23,7 @@ final class DifferentialHunkTestCase extends ArcanistPhutilTestCase { ); $this->assertEqual($added, $hunk->getAddedLines()); - $hunk = new DifferentialHunk(); + $hunk = new DifferentialHunkLegacy(); $hunk->setChanges(Filesystem::readFile($root.'newline.diff')); $hunk->setOldOffset(1); $hunk->setNewOffset(11); diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 755435fad4..c12bdd5e0e 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -382,8 +382,8 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker // -echo "test"; // -(empty line) - $hunk = id(new DifferentialHunk())->setChanges($context); - $vs_hunk = id(new DifferentialHunk())->setChanges($vs_context); + $hunk = id(new DifferentialHunkLegacy())->setChanges($context); + $vs_hunk = id(new DifferentialHunkLegacy())->setChanges($vs_context); if ($hunk->makeOldFile() != $vs_hunk->makeOldFile() || $hunk->makeNewFile() != $vs_hunk->makeNewFile()) { return $vs_diff; -- 2.51.2