Skip unnecessary cache updates

[individual-it]

OC\Files\Cache\Updater::update does a couple of updates. Not all are needed in every case the method is called.
In this example if touch() only is changing the mtime we should skip the update of the folder size as that would not change.
In a short test this fix saves around 7-8% time when uploading small files. #7072

[icewind1991]

I would prefer to have a seperate update method for only changing mtimes instead of adding a parameter

[individual-it]

then we should have:
updateMtime($storage, $internalPath)
updateFolderSize($storage, $internalPath)
update($path, $time = null) - that call both above

so you could call one or the other or both. Need to make sure if called both that propagateChanges($time); only runs once

[icewind1991]

when doing a chunked upload $this->path and $targetPath are not the same

[individual-it]

sorry that change had to go into put() and not into createFileChunked()
should be fixed now

[icewind1991]

cc @PVince81

[scrutinizer-notifier]

A new inspection was created.

[individual-it]

I have done some more serious tests. Running 4 user sessions, every uploading 10 folders with 10 small files (4byte) in it. After the upload the test runs PROFIND and downloads all the files again.
Here is the code of the test: https://github.com/individual-it/owncloud-performance-test
It depends on https://github.com/owncloud/pyocclient/

I have compared:

• 8.1
• the master branch of the state before I've branched out
  73b43db
• this patch (https://github.com/individual-it/core/tree/skip_unnecessary_cache_updates).

And that is what I got after running the test 3 times and calculating the means:

[icewind1991]

👍

[individual-it]

@karlitschek can we put this to 8.2 milestone?

[karlitschek]

@DeepDiver1975 @PVince81 @schiesbn opinions?

[PVince81]

Need to take some time to test whether syncing still works, and also with SMB which doesn't support touch.

[PVince81]

By the way, @individual-it there are actually two code paths for file uploads.
The one you changed is for non-chunked upload.
See here the part for chunked upload: https://github.com/owncloud/core/blob/v8.1.3/lib/private/connector/sabre/file.php#L386 (yay, duplicate codez)
file_assemble is here: https://github.com/owncloud/core/blob/master/lib/private/filechunking.php#L188

Not sure if that part needs to be adjusted too ?
It doesn't seem to call update() there, hmmm suspicious.
@icewind1991 can you comment ?

[individual-it]

In the case of a chunked file the OC\Files\Cache\Updater::update() is called by store(): https://github.com/owncloud/core/blob/master/lib/private/connector/sabre/file.php#L350

stack:

owncloud-core/lib/private/files/cache/updater.php.OC\Files\Cache\Updater->update(): lineno 110  
owncloud-core/lib/private/files/view.php.OC\Files\View->rename(): lineno 697    
owncloud-core/lib/private/cache/file.php.OC\Cache\File->set(): lineno 115   
owncloud-core/lib/private/filechunking.php.OC_FileChunking->store(): lineno 71  
owncloud-core/lib/private/connector/sabre/file.php.OC\Connector\Sabre\File->createFileChunked(): lineno 338 

I guess it make sense to update the parent folder size after every chunk got uploaded. We could theoretically skip the last update when HTTP_X_OC_MTIME is set and a "touch" has to be run. But on big files the performance improvement will not be huge. It really matters only on a big number of small files where you can save some DB requests for every upload.

btw. your pyoclient is a great tool to debug.
e.g. oc.put_file('testfolder/test/test/1','big-file',keep_mtime=False) uploads a chunked file without setting HTTP_X_OC_MTIME

[PVince81]

I guess it make sense to update the parent folder size after every chunk got uploaded.

I don't think it is supposed to do that. The chunks are uploaded into the "cache" folder, not the final folder, so there is nothing to update there. I guess it just does nothing then.
It's only when assembling the final file in "file_assemble" that the part file gets created, and then eventually renamed to the final file. At this time the size propagation makes sense.

[PVince81]

There must be another later call to update() from within file_assemble

[individual-it]

last call I can see in the debugger is by rename()

owncloud-core/lib/private/files/cache/updater.php.OC\Files\Cache\Updater->update(): lineno 111  
owncloud-core/lib/private/files/view.php.OC\Files\View->rename(): lineno 697    
owncloud-core/lib/private/connector/sabre/file.php.OC\Connector\Sabre\File->createFileChunked(): lineno 365 
owncloud-core/lib/private/connector/sabre/file.php.OC\Connector\Sabre\File->put(): lineno 100   

[schiessle]

then we should have:
updateMtime($storage, $internalPath)
updateFolderSize($storage, $internalPath)
update($path, $time = null) - that call both above

Doesn't this mean that we now iterate twice over the file tree if we need to update mtime and size while before it was done in one run? Doesn't sound like a good idea just to improve the touch, in most cases we will probably have to update both. Or do I miss something?

[individual-it]

that's why there is the $skipPropagatingChanges switch.
If you need to run both you just run update() and that will skip one propagation
besides of the "if" checks
the only doubled code is list($storage, $internalPath) = $this->view->resolvePath($path);

[DeepDiver1975]

superseded by #19494