drupal_move_uploaded_file() currently attempts to call move_uploaded_file() with an unresolved destination URI. If this fails it will fall back to passing a resolved URI. This is to support safe_mode and open_basedir.
If drupal_move_uploaded_file() is passed an unresolved URI it will perform a file copy followed by an unlink. This produces a considerable amount of I/O. If it is passed a resolved URI (a filepath) it will perform a file rename which produces negligible I/O.
The attached patch means that it always attempts to resolve the destination URI that is passed to move_uploaded_file(). If the URI does not resolve (it is a remote scheme) then move_uploaded_file() will fall back to copying and deleting. This will not cause write I/O because the scheme is remote.
I tested the performance changes of the patch by uploading an Ubuntu .iso file to a file field and measuring the I/O using
sudo iotop -Pao
Without the patch it takes 1390.60 MB of writes.
With the patch it takes 695.30 MB of writes (the file is only written to disk once).
On a loaded server the improvement would be even more. During the testing, my laptop had sufficient free RAM for the written file to be cached in RAM. This meant it could be written to the destination without having to be read. If there was no free RAM there would have to be 695.30 MB of reads.
So - this patch reduces the amount of I/O required for a file upload by 50 - 66 %.
There are already extensive tests for uploading files to local and remote URI schemes.
Comment | File | Size | Author |
---|---|---|---|
#43 | 1395524-43.patch | 856 bytes | Ankit.Gupta |
#40 | reroll_diff_28-40.txt | 2.67 KB | immaculatexavier |
#40 | 1395524-40.patch | 850 bytes | immaculatexavier |
#28 | 1395524-28.patch | 1.65 KB | MerryHamster |
#24 | optimize-file-uploading-1395524-24.patch | 1.65 KB | savkaviktor16@gmail.com |
Issue fork drupal-1395524
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
droplet CreditAttribution: droplet commentedIs there any chance $destination is FALSE ? maybe add an IF check.
-5 days to next Drupal core point release.
Comment #2
jbrown CreditAttribution: jbrown commentedThe ?: operator ensures that if drupal_realpath($uri) is FALSE then just $uri is returned.
Comment #4
jbrown CreditAttribution: jbrown commentedoptimize-file-uploading.patch queued for re-testing.
Comment #5
jbrown CreditAttribution: jbrown commentedReroll.
Note: I retested the performance of this patch, but discovered that the optimization was already being performed. This was because I am now using Btrfs on my laptop and it has copy-on-write. Re-testing on Ext4, the patch performs in line with the results in the issue summary.
See also #1377740: file_unmanaged_move() should issue rename() where possible instead of copy() & unlink().
Comment #6
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #7
Dave ReidComment #8
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #9
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #10
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe claim about COW is dubious, but the patch makes good sense.
Before committing here, could we open a bug report against PHP so that they include trying to resolve URIs first in their filesystem operations?
Comment #12
jbrown CreditAttribution: jbrown commentedhttps://bugs.php.net/bug.php?id=64178
Comment #13
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #14
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #15
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #16
jbrown CreditAttribution: jbrown commented#5: optimize-file-uploading.patch queued for re-testing.
Comment #23
volegerComment #24
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedre-rolled
Comment #25
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #26
MerryHamster CreditAttribution: MerryHamster at Skilld commentedreroll for 8.6.x
Comment #27
volegerNeeds code indentation fix.
Comment #28
MerryHamster CreditAttribution: MerryHamster at Skilld commentedfixed #27
Comment #29
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #30
volegerShould not this be fixed directly in
moveUploadedFile
method offile_system
service?Comment #38
larowlanAs voleger points out, this is now part of file system service
Comment #40
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled #28 patch against 9.5.x
Attached Rerolled diff
Comment #43
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedReroll the patch #40 with Drupal 10.1.x