From 7ecf9ab87eb8ede5af54dbab5b8b2579fba66231 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 18 Nov 2012 02:32:50 -0800 Subject: [PATCH] Various simple optimizations for the chunked upload process. * This adds an UnregisteredLocalFile::setLocalReference() function, which is used to avoid an extra GET request. * This removes the useless rename() in stashFile(). We just need to make sure the stashed file has the right extension. Getting rid of the rename makes setLocalReference() usable. * Also adds some debug logging with ellapsed time. Change-Id: I087701ad0c27a4eba74591e6b49f5667b011424c --- includes/filerepo/FileRepo.php | 11 +++++++++++ .../filerepo/file/UnregisteredLocalFile.php | 18 +++++++++++++----- includes/upload/UploadBase.php | 2 +- includes/upload/UploadFromChunks.php | 13 ++++++++++++- includes/upload/UploadStash.php | 15 +++++---------- 5 files changed, 42 insertions(+), 17 deletions(-) diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 651ee27127..7bcadd891f 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -1405,6 +1405,17 @@ class FileRepo { return $this->backend->getFileTimestamp( array( 'src' => $path ) ); } + /** + * Get the size of a file with a given virtual URL/storage path + * + * @param $virtualUrl string + * @return integer|bool False on failure + */ + public function getFileSize( $virtualUrl ) { + $path = $this->resolveToStoragePath( $virtualUrl ); + return $this->backend->getFileSize( array( 'src' => $path ) ); + } + /** * Get the sha1 (base 36) of a file with a given virtual URL/storage path * diff --git a/includes/filerepo/file/UnregisteredLocalFile.php b/includes/filerepo/file/UnregisteredLocalFile.php index 8d4a3f88e0..698d1ebe78 100644 --- a/includes/filerepo/file/UnregisteredLocalFile.php +++ b/includes/filerepo/file/UnregisteredLocalFile.php @@ -179,10 +179,18 @@ class UnregisteredLocalFile extends File { */ function getSize() { $this->assertRepoDefined(); - $props = $this->repo->getFileProps( $this->path ); - if ( isset( $props['size'] ) ) { - return $props['size']; - } - return false; // doesn't exist + return $this->repo->getFileSize( $this->path ); + } + + /** + * Optimize getLocalRefPath() by using an existing local reference. + * The file at the path of $fsFile should not be deleted (or at least + * not until the end of the request). This is mostly a performance hack. + * + * @param $fsFile FSFile + * @return void + */ + public function setLocalReference( FSFile $fsFile ) { + $this->fsFile = $fsFile; } } diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php index d40b53d3a1..fa4931cfcf 100644 --- a/includes/upload/UploadBase.php +++ b/includes/upload/UploadBase.php @@ -244,7 +244,7 @@ abstract class UploadBase { // @TODO: just make uploads work with storage paths // UploadFromStash loads files via virtuals URLs $tmpFile = $repo->getLocalCopy( $srcPath ); - $tmpFile->bind( $this ); // keep alive with $thumb + $tmpFile->bind( $this ); // keep alive with $this wfProfileOut( __METHOD__ ); return $tmpFile->getPath(); } diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php index b0e5fb6c71..e923c20783 100644 --- a/includes/upload/UploadFromChunks.php +++ b/includes/upload/UploadFromChunks.php @@ -120,17 +120,24 @@ class UploadFromChunks extends UploadFromFile { // Get a 0-byte temp file to perform the concatenation at $tmpFile = TempFSFile::factory( 'chunkedupload_', $ext ); $tmpPath = $tmpFile - ? $tmpFile->getPath() + ? $tmpFile->bind( $this )->getPath() // keep alive with $this : false; // fail in concatenate() // Concatenate the chunks at the temp file + $tStart = microtime( true ); $status = $this->repo->concatenate( $fileList, $tmpPath, FileRepo::DELETE_SOURCE ); + $tAmount = microtime( true ) - $tStart; if( !$status->isOk() ){ return $status; } + wfDebugLog( 'fileconcatenate', "Combined $i chunks in $tAmount seconds.\n" ); // Update the mTempPath and mLocalFile // ( for FileUpload or normal Stash to take over ) $this->mTempPath = $tmpPath; // file system path + $tStart = microtime( true ); $this->mLocalFile = parent::stashFile(); + $tAmount = microtime( true ) - $tStart; + $this->mLocalFile->setLocalReference( $tmpFile ); // reuse (e.g. for getImageInfo()) + wfDebugLog( 'fileconcatenate', "Stashed combined file ($i chunks) in $tAmount seconds.\n" ); return $status; } @@ -203,6 +210,9 @@ class UploadFromChunks extends UploadFromFile { $this->getOffset() . ' inx:' . $this->getChunkIndex() . "\n" ); $dbw = $this->repo->getMasterDb(); + // Use a quick transaction since we will upload the full temp file into shared + // storage, which takes time for large files. We don't want to hold locks then. + $dbw->begin(); $dbw->update( 'uploadstash', array( @@ -213,6 +223,7 @@ class UploadFromChunks extends UploadFromFile { array( 'us_key' => $this->mFileKey ), __METHOD__ ); + $dbw->commit(); } /** diff --git a/includes/upload/UploadStash.php b/includes/upload/UploadStash.php index 733c686680..09bcaeafd4 100644 --- a/includes/upload/UploadStash.php +++ b/includes/upload/UploadStash.php @@ -182,7 +182,7 @@ class UploadStash { * @return UploadStashFile: file, or null on failure */ public function stashFile( $path, $sourceType = null ) { - if ( ! file_exists( $path ) ) { + if ( !is_file( $path ) ) { wfDebug( __METHOD__ . " tried to stash file at '$path', but it doesn't exist\n" ); throw new UploadStashBadPathException( "path doesn't exist" ); } @@ -192,12 +192,10 @@ class UploadStash { // we will be initializing from some tmpnam files that don't have extensions. // most of MediaWiki assumes all uploaded files have good extensions. So, we fix this. $extension = self::getExtensionForPath( $path ); - if ( ! preg_match( "/\\.\\Q$extension\\E$/", $path ) ) { + if ( !preg_match( "/\\.\\Q$extension\\E$/", $path ) ) { $pathWithGoodExtension = "$path.$extension"; - if ( ! rename( $path, $pathWithGoodExtension ) ) { - throw new UploadStashFileException( "couldn't rename $path to have a better extension at $pathWithGoodExtension" ); - } - $path = $pathWithGoodExtension; + } else { + $pathWithGoodExtension = $path; } // If no key was supplied, make one. a mysql insertid would be totally reasonable here, except @@ -221,7 +219,7 @@ class UploadStash { wfDebug( __METHOD__ . " key for '$path': $key\n" ); // if not already in a temporary area, put it there - $storeStatus = $this->repo->storeTemp( basename( $path ), $path ); + $storeStatus = $this->repo->storeTemp( basename( $pathWithGoodExtension ), $path ); if ( ! $storeStatus->isOK() ) { // It is a convention in MediaWiki to only return one error per API exception, even if multiple errors @@ -244,9 +242,6 @@ class UploadStash { } $stashPath = $storeStatus->value; - // we have renamed the file so we have to cleanup once done - unlink($path); - // fetch the current user ID if ( !$this->isLoggedIn ) { throw new UploadStashNotLoggedInException( __METHOD__ . ' No user is logged in, files must belong to users' ); -- 2.20.1