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

diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index 651ee27..7bcadd8 100644 (file)
--- 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 8d4a3f8..698d1eb 100644 (file)
--- 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 d40b53d..fa4931c 100644 (file)
--- 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 b0e5fb6..e923c20 100644 (file)
--- 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 733c686..09bcaea 100644 (file)
--- 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' );