Optimize PUT - with custom mtime, use storage instead of view #28003

mrow4a · 2017-05-24T21:28:59Z

Description

As in the title, I am trying to avoid unnecessarily queries delivering this information different way then just querying the database twice.

This PR saves few queries in the tested example, 4 READ and 4 WRITE (!), needs 2 aditional LOCK updates, so +2 WRITE.

Integrations tests in place for this feature
Might need some unit test update?
In other PR, I am doing the same with checksum, and this PR might need adjustment after merge of Optimize PUT - don't fetch and update checksum again, reunse the one from part file #27532

PR reduces the number of queries from 92 to 82 for new, and from 111 to 101 for edited file (SELECTS and UPDATES)

How did I test

Smashbox basic test

docker run -e SMASHBOX_URL=172.42.16.124:80/octest -e SMASHBOX_USERNAME=admin -e SMASHBOX_ACCOUNT_PASSWORD=admin -e SMASHBOX_PASSWORD=admin owncloud/smashbox lib/test_basicSync.py

Using diagnostic, compared number of queries

Previous impl - single put new file

{"type":"SUMMARY","reqId":"0S1oOpIXAOJaU7i66fAL","time":"2018-05-16T15:19:45+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":92,"totalSQLDurationmsec":15.33818244934082,"totalSQLParams":205,"totalEvents":16,"totalEventsDurationmsec":44.87967491149902}}

Previous impl - single put edit file

{"type":"SUMMARY","reqId":"QxoT5rglLIWSHBJl3XRb","time":"2018-05-16T15:19:45+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":111,"totalSQLDurationmsec":17.354965209960938,"totalSQLParams":259,"totalEvents":16,"totalEventsDurationmsec":59.42058563232422}}

Current impl - single put new file

{"type":"SUMMARY","reqId":"5L9J79Am5wkkeqfKY7Wr","time":"2018-05-16T15:21:18+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":82,"totalSQLDurationmsec":13.219833374023438,"totalSQLParams":175,"totalEvents":16,"totalEventsDurationmsec":58.83908271789551}}

Current impl - single put edit file

{"type":"SUMMARY","reqId":"AWPSyMmw8aCWHDmHnKZ0","time":"2018-05-16T15:21:18+00:00","remoteAddr":"::1","user":"admin","method":"PUT","url":"/octest/remote.php/dav/files/admin/testnew.txt","diagnostics":{"totalSQLQueries":101,"totalSQLDurationmsec":15.613555908203125,"totalSQLParams":223,"totalEvents":16,"totalEventsDurationmsec":50.08506774902344}}

mrow4a · 2017-05-24T21:32:24Z

@IljaN @tomneedham @DeepDiver1975 Thoughts?

DeepDiver1975 · 2017-05-26T07:48:16Z

lib/private/Files/Storage/Common.php

@@ -76,7 +77,13 @@
 	protected $mountOptions = [];
 	protected $owner = null;

+	/** @var CappedMemoryCache Key is path, value is array of metadata key-value pairs */
+	private static $metadata;


why static?

Hmm, seems this wrapper wont need it static, seems you are right.

DeepDiver1975 · 2017-05-26T07:49:32Z

apps/dav/lib/Connector/Sabre/File.php

+			// allow sync clients to send the mtime along in a header
+			$request = \OC::$server->getRequest();
+			if (isset($request->server['HTTP_X_OC_MTIME'])) {
+				$storage->addMetaData($internalPath, 'mtime', $request->server['HTTP_X_OC_MTIME']);


where is the actual write operation of the meta data?

Will add description to that. It does not write any metadata hardly "to disk", any new call to getMetadata (e.g. in ->update) will have metadata overwritten with this.

Maybe I should call it cacheMetadata or sth?

@DeepDiver1975

but the touch operation you are replacing was storing the mtime in file cache - my question is where this storing of information is happening now

DeepDiver1975 · 2017-05-26T07:49:51Z

lib/private/Files/Storage/Storage.php

+	 * @param string $key
+	 * @param string $val
+	 */
+	public function addMetaData($path, $key, $val);


$val -> $value

mrow4a · 2017-05-26T07:55:01Z

@DeepDiver1975 I will try also to reuse it for checksums, to avoid more queries -> https://github.com/owncloud/core/pull/28003/files#diff-be504dc886734275f06114e4306fa215L231

mrow4a · 2017-05-26T10:36:17Z

Ok, I see this wont be that easy, since "touch" literally touches the file, and this needs to be done separately @DeepDiver1975 .. ehh. This not only affects database but mtime of existing file on disk.

mrow4a · 2017-05-26T14:36:55Z

apps/dav/lib/Connector/Sabre/File.php

 			// allow sync clients to send the mtime along in a header
 			$request = \OC::$server->getRequest();
 			if (isset($request->server['HTTP_X_OC_MTIME'])) {
+				// Touch will update mtime of file at storage, scan and emit the hooks
 				if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) {


The only problem which I see here, that touch() without previous call to update() will pass to preTouch hook, NonExistingNode ->

core/lib/private/Files/Node/HookConnector.php

Line 110 in f10d105

$this->root->emit('\OC\Files', 'preTouch', [$node]);

Question is, what do we need it for and if it is ok to call touch() on non-existing yet node.

In case of update, pretouch will receive "previous version"

PostTouch is executed after the scan, and receives new, correct node :>

For now, I dont see anything using that https://github.com/search?q=org%3Aowncloud+preTouch&type=Code, delete this hook ? @DeepDiver1975 @PVince81 ?

These hooks could be used by third party apps so deleting would be API breakage.

why is this a problem ?

This particular preTouch adds additional queries (WRITEs/READs/UPDATEs) to each PUT files, does not matter if that app is using your pre-touch or not.

When you apply the solution above, you get rid of some queries.. when you remove preTouch you get rid of additional queries needed for LOCK, since this preTouch does locking+filecache query.

Problem with solution above is that we pass different things to preTouch, not sure if the correct ones. Will be good to know what app using preTouch requires in the input @DeepDiver1975 @PVince81

If we delete pretouch, would there be an alternative hook for developers to listen to ? Is there another hook ?

Would require informing the developers mailing list and wait a bit for feedback.
If we remove it, it would be in 10.1 and not backportable (which is fine).

@PVince81 So, do we remove preTouch? :>

PVince81 · 2017-07-04T10:08:57Z

I don't think we can remove the "touch" hook which might be used by third party app, that would be API breakage. So we might need to abandon this optimization.

@DeepDiver1975 agree ? or kill the hook ?

mrow4a · 2017-07-27T13:49:31Z

preTouch hook 🔫 ? @DeepDiver1975 @PVince81 @tomneedham

mrow4a · 2017-08-03T08:24:02Z

Any update on pretouch ?

PVince81 · 2017-08-03T08:29:47Z

Any update on pretouch ?

We can't kill it without notifying the community.
Also I don't feel good about killing this in a minor release (10.0.3).

@DeepDiver1975 thoughts ?

PVince81 · 2017-08-10T07:28:29Z

moving to "planned", blocked by the decision of deprecating/removing pre/post touch hooks

PVince81 · 2018-02-15T15:54:05Z

can we have this PR without killing the touch event ?

mrow4a · 2018-05-16T15:34:31Z

lib/private/Files/View.php

@@ -313,6 +313,10 @@ protected function removeMount($mount, $path) {
 		}
 	}

+	public function isCacheUpdateEnabled() {


Will remove, it should be in previous commit.

mrow4a · 2018-05-16T15:35:00Z

apps/dav/lib/Connector/Sabre/File.php

+	 * @param Storage $targetStorage
+	 * @param $targetInternalPath
+	 */
+	private function handleMetadataUpdate(\OC\Files\Storage\Storage $targetStorage, $targetInternalPath) {


now we use the same function to commit mtime and propagate update - both in chunked and in single PUT

PVince81

👍

DeepDiver1975 · 2018-05-17T12:40:00Z

@ownclouders rebase

ownclouders · 2018-05-17T12:40:02Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

DeepDiver1975 · 2018-05-18T08:09:45Z

looks like rebasing with gitmate does not work anymore ...

mrow4a · 2018-05-19T16:56:34Z

It was with newest master when you rebased I think.

mrow4a · 2018-05-20T16:36:09Z

apps/dav/lib/Connector/Sabre/File.php

+		}
+
+		// since we skipped the view we need to scan and emit the hooks ourselves
+		$targetStorage->getUpdater()->update($targetInternalPath);


@PVince81 I am thinking, should we pass to update function mtime, or not? I cannot scratch my head over the case in which mtime wont be updated (updater should detect it I think)

Maybe for safety we should call updater with mtime in case HTTP_X_OC_MTIME is set, and without in case it is not, thoughts?

maybe pass it in explicitly so it involves less magic. magic being "remembering that the function is reading a global var / header"

use target storage update instead

mrow4a · 2018-06-25T20:21:12Z

@DeepDiver1975 @PVince81 ok, I think finally this is ready :>

mrow4a · 2018-06-28T18:12:37Z

apps/dav/lib/Connector/Sabre/File.php

+			$mtime = $this->sanitizeMtime(
+				$this->request->server ['HTTP_X_OC_MTIME']
+			);
+			if ($targetStorage->touch($targetInternalPath, $mtime)) {


If someone wonders what essential changes, it is that we now use $targetStorage->touch & $targetStorage->getUpdater()->update($targetInternalPath, $mtime) instead of $this->fileView->touch (which calls updater update() internally) & $targetStorage->getUpdater()->update($targetInternalPath, $mtime).

Thus we call update 1x, instead 2x

lock · 2019-07-30T13:02:37Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mrow4a added the 2 - Developing label May 24, 2017

mrow4a requested review from PVince81 and DeepDiver1975 May 24, 2017 21:28

mrow4a changed the title ~~Set mtime from MemoryCache instead of using touch()~~ Optimize PUT - set mtime from cache instead of using touch() May 25, 2017

DeepDiver1975 requested changes May 26, 2017

View reviewed changes

mrow4a force-pushed the optimize_put_5 branch from c132264 to b2d36ba Compare May 26, 2017 14:34

mrow4a commented May 26, 2017

View reviewed changes

mrow4a changed the title ~~Optimize PUT - set mtime from cache instead of using touch()~~ Optimize PUT - with custom mtime, reuse touch() for runnig update() May 26, 2017

mrow4a force-pushed the optimize_put_5 branch from b2d36ba to 0469636 Compare May 27, 2017 12:56

mrow4a mentioned this pull request Jun 21, 2017

Optimize PUT - don't fetch and update checksum again, reunse the one from part file #27532

Merged

1 task

mrow4a force-pushed the optimize_put_5 branch from 0469636 to dfbb4e6 Compare August 3, 2017 08:30

PVince81 assigned mrow4a Aug 7, 2017

PVince81 added this to the planned milestone Aug 10, 2017

PVince81 modified the milestones: development, planned Nov 2, 2017

PVince81 modified the milestones: planned, development Nov 22, 2017

ownclouders added the status/STALE label Dec 7, 2017

PVince81 modified the milestones: development, planned Jan 12, 2018

ownclouders removed the status/STALE label Feb 15, 2018

mrow4a commented May 16, 2018

View reviewed changes

mrow4a force-pushed the optimize_put_5 branch from 5bec6c3 to a433506 Compare May 16, 2018 15:40

mrow4a changed the title ~~Optimize PUT - with custom mtime, reuse touch() for running update()~~ Optimize PUT - with custom mtime, use storage instead of view May 16, 2018

PVince81 approved these changes May 17, 2018

View reviewed changes

mrow4a commented May 20, 2018

View reviewed changes

PVince81 modified the milestones: triage, maybe some day May 22, 2018

ownclouders added the status/STALE label May 30, 2018

mrow4a force-pushed the optimize_put_5 branch from a433506 to 6bcc843 Compare June 13, 2018 18:43

ownclouders removed the status/STALE label Jun 13, 2018

mrow4a force-pushed the optimize_put_5 branch 4 times, most recently from d2fa99f to 5e797f4 Compare June 17, 2018 08:46

ownclouders added the status/STALE label Jun 25, 2018

Dont update again if mtime is to be updated,

17b3608

use target storage update instead

mrow4a force-pushed the optimize_put_5 branch from 5e797f4 to 17b3608 Compare June 25, 2018 18:50

ownclouders removed the status/STALE label Jun 25, 2018

DeepDiver1975 merged commit 2de709e into master Jun 25, 2018

DeepDiver1975 deleted the optimize_put_5 branch June 25, 2018 20:24

mrow4a mentioned this pull request Jun 25, 2018

[stable10] Optimize PUT - with custom mtime, use storage instead of view #31891

Merged

mrow4a commented Jun 28, 2018

View reviewed changes

lock bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Optimize PUT - with custom mtime, use storage instead of view #28003

Optimize PUT - with custom mtime, use storage instead of view #28003

Conversation

mrow4a commented May 24, 2017 • edited Loading

Description

How did I test

Previous impl - single put new file

Previous impl - single put edit file

Current impl - single put new file

Current impl - single put edit file

mrow4a commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented May 26, 2017

mrow4a commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a May 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 4, 2017

mrow4a commented Jul 27, 2017 • edited Loading

mrow4a commented Aug 3, 2017

PVince81 commented Aug 3, 2017

PVince81 commented Aug 10, 2017

PVince81 commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

DeepDiver1975 commented May 17, 2018

ownclouders commented May 17, 2018

DeepDiver1975 commented May 18, 2018

mrow4a commented May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Jun 25, 2018

Choose a reason for hiding this comment

lock bot commented Jul 30, 2019

mrow4a commented May 24, 2017 •

edited

Loading

mrow4a May 26, 2017 •

edited

Loading

mrow4a May 30, 2017 •

edited

Loading

mrow4a commented Jul 27, 2017 •

edited

Loading

mrow4a commented May 19, 2018 •

edited

Loading