From c19bb57730a26d844dee191f738b2616c4c7f9af Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 14 Mar 2015 08:29:30 -0700 Subject: [PATCH] Stream chunks when sending chunked files Summary: Ref T7149. Return a real iterator from the Chunk engine, which processes chunks sequentially. Test Plan: This is a bit hard to read, but shows the underlying chunks being accessed one at a time and only some being accessed when requesting a range of a file: ``` $ ./bin/files cat F878 --trace --begin 100 --end 256 ... >>> [10] SELECT * FROM `file_storageblob` WHERE `id` = 85 <<< [10] 240 us better software. Phabricat>>> [11] SELECT * FROM `file_storageblob` WHERE `id` = 84 <<< [11] 205 us or includes applications for: >>> [12] SELECT * FROM `file_storageblob` WHERE `id` = 83 <<< [12] 226 us - reviewing and auditing source>>> [13] SELECT * FROM `file_storageblob` WHERE `id` = 82 <<< [13] 203 us code; - hosting and browsing >>> [14] SELECT * FROM `file_storageblob` WHERE `id` = 81 <<< [14] 231 us repositories; - tracking bugs; ``` Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T7149 Differential Revision: https://secure.phabricator.com/D12073 --- src/__phutil_library_map__.php | 5 ++ .../PhabricatorChunkedFileStorageEngine.php | 11 +++ .../engine/PhabricatorFileChunkIterator.php | 72 +++++++++++++++++++ .../engine/PhabricatorFileStorageEngine.php | 15 ++++ .../files/storage/PhabricatorFile.php | 15 +--- 5 files changed, 105 insertions(+), 13 deletions(-) create mode 100644 src/applications/files/engine/PhabricatorFileChunkIterator.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 32407404a3..97c19121e0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1795,6 +1795,7 @@ phutil_register_library_map(array( 'PhabricatorFile' => 'applications/files/storage/PhabricatorFile.php', 'PhabricatorFileBundleLoader' => 'applications/files/query/PhabricatorFileBundleLoader.php', 'PhabricatorFileChunk' => 'applications/files/storage/PhabricatorFileChunk.php', + 'PhabricatorFileChunkIterator' => 'applications/files/engine/PhabricatorFileChunkIterator.php', 'PhabricatorFileChunkQuery' => 'applications/files/query/PhabricatorFileChunkQuery.php', 'PhabricatorFileCommentController' => 'applications/files/controller/PhabricatorFileCommentController.php', 'PhabricatorFileComposeController' => 'applications/files/controller/PhabricatorFileComposeController.php', @@ -5096,6 +5097,10 @@ phutil_register_library_map(array( 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', ), + 'PhabricatorFileChunkIterator' => array( + 'Phobject', + 'Iterator', + ), 'PhabricatorFileChunkQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorFileCommentController' => 'PhabricatorFileController', 'PhabricatorFileComposeController' => 'PhabricatorFileController', diff --git a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php index 641e6658b2..5240dd4630 100644 --- a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php @@ -177,4 +177,15 @@ final class PhabricatorChunkedFileStorageEngine return 32; } + public function getFileDataIterator(PhabricatorFile $file, $begin, $end) { + $chunks = id(new PhabricatorFileChunkQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withChunkHandles(array($file->getStorageHandle())) + ->withByteRange($begin, $end) + ->needDataFiles(true) + ->execute(); + + return new PhabricatorFileChunkIterator($chunks, $begin, $end); + } + } diff --git a/src/applications/files/engine/PhabricatorFileChunkIterator.php b/src/applications/files/engine/PhabricatorFileChunkIterator.php new file mode 100644 index 0000000000..50b605dc95 --- /dev/null +++ b/src/applications/files/engine/PhabricatorFileChunkIterator.php @@ -0,0 +1,72 @@ +chunks = $chunks; + + if ($begin !== null) { + foreach ($chunks as $key => $chunk) { + if ($chunk->getByteEnd() >= $begin) { + unset($chunks[$key]); + } + break; + } + $this->begin = $begin; + } + + if ($end !== null) { + foreach ($chunks as $key => $chunk) { + if ($chunk->getByteStart() <= $end) { + unset($chunks[$key]); + } + } + $this->end = $end; + } + } + + public function current() { + $chunk = head($this->chunks); + $data = $chunk->getDataFile()->loadFileData(); + + if ($this->end !== null) { + if ($chunk->getByteEnd() > $this->end) { + $data = substr($data, 0, ($this->end - $chunk->getByteStart())); + } + } + + if ($this->begin !== null) { + if ($chunk->getByteStart() < $this->begin) { + $data = substr($data, ($this->begin - $chunk->getByteStart())); + } + } + + return $data; + } + + public function key() { + return head_key($this->chunks); + } + + public function next() { + unset($this->chunks[$this->key()]); + } + + public function rewind() { + return; + } + + public function valid() { + return (count($this->chunks) > 0); + } + +} diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php index f519874a16..8a9749f9dc 100644 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -290,5 +290,20 @@ abstract class PhabricatorFileStorageEngine { return $engine->getChunkSize(); } + public function getFileDataIterator(PhabricatorFile $file, $begin, $end) { + // The default implementation is trivial and just loads the entire file + // upfront. + $data = $file->loadFileData(); + + if ($begin !== null && $end !== null) { + $data = substr($data, $begin, ($end - $begin)); + } else if ($begin !== null) { + $data = substr($data, $begin); + } else if ($end !== null) { + $data = substr($data, 0, $end); + } + + return array($data); + } } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 2fbcf3a891..aa681f3233 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -612,19 +612,8 @@ final class PhabricatorFile extends PhabricatorFileDAO * @return Iterable Iterable object which emits requested data. */ public function getFileDataIterator($begin = null, $end = null) { - // The default implementation is trivial and just loads the entire file - // upfront. - $data = $this->loadFileData(); - - if ($begin !== null && $end !== null) { - $data = substr($data, $begin, ($end - $begin)); - } else if ($begin !== null) { - $data = substr($data, $begin); - } else if ($end !== null) { - $data = substr($data, 0, $end); - } - - return array($data); + $engine = $this->instantiateStorageEngine(); + return $engine->getFileDataIterator($this, $begin, $end); }