Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 #5440

mrow4a · 2017-01-08T23:12:48Z

This fixes issues:
#5391 In the mixed file size scenario bandwidth is very underutilized - enterprise setup
#5390 Broken sync time estimation - enterprise setup
#4498 sync files from smallest first to biggest last
#1633 Propagator: Balance concurrent up/down-load
#4454 Investigate HTTP Pipelining

This includes:
#5400 so if this is merged, the other can be closed

This is basic for:
#5319 Bundling
HTTP2

This can be enhanced by:
#5368 Dynamic Chunking
#5349 Sorting folders by modification time

This requires following capability on server side, otherwise will sync old way:

Idea

The idea behind the implemented algorithm is that in the scenario, in which we have a lot of mixed within the same or different folder - big files, under chunking size files of size 100kB or 5MB, files to download, moves, deletes, directory creations etc:

Separate transfer jobs from jobs which server database interaction is major factor
Reserve uncoditionaly 4-6 flows for "db sensitive files" like small file uploads and 2 for upload/download >1MB file transfers
sync normalny if no other files
Cross folder, this means it will look for "db sensitive" and "data transfer" in all folders.

The above will ensure that there is a balance between bandwidth utilization and bookkeeping, what in turn gives faster synchronisation (for details look below)

Used algorithm:

Checkpoints

Cross folder, this means it will look for "db sensitive" and "data transfer" in all folders.
Test manualy to ensure faster sync - passed prove of concept
[x ] Test on enterprise setup to ensure faster sync
Write unit tests to cover whole the code
Test with integration tests

Details

Lets do some maths. This will be our folder to sync:
100 files - average 100kB file size -> total 10MB to be transfered
10 files - average 10MB file size -> total 100MB to be transfered

Assume, that your network is 5MB/s and one "1 Byte PUT" takes 1s (we are not in ideal home case scenario with empty server now guys, it is not that easy, it could take much much more)

The request "latency" consists of 2 components, time it takes for bookkeeping on server and time it takes for data transfer.

Current case

If you do 100 small request in the row, your data transfer is neglible ~0s and bookkeeping time is 100s. Parallelising that maybe you can achieve 33s having 3 parallel flows.

If you do 10 biger files request in a row, your data transfer is 10s bookkeeping. Parallelised say 4s if you are lucky with 3 parallel flows. However, you cannot omit 20s coming from transfer of 100MB, does not matter how many requests you have in parallel 1000 or 1. Your 5MB/s office net bounds you.

Total you need around 33s plus 24s from big, having 57s.

Optimized case

If you do 100 small request in the row, your data transfer is neglible ~0s and bookkeeping time is 100s. Parallelising that maybe you can achieve 50s reserving 2 flow slots. In this 50s, you used neglible bandwidth. If you use the 3rd flow to pump there 30s comming from big files, you just synced your files in 50s, since your 30s are done in parallel filling the bandwidth.

57s vs 50s is like 13% of the time orginal time? In this example it is around 7s. 100 files is not a big deal but shows bigger picture

More big files and more small files, the different is the percentage. Do the math for 55000 files and 55GB. Where you have 40000 of small files in 10GB(avg. size 250kB) and 15000 in the rest 45GB (avg size 3MB). I think we wont be talking about even minutes there :>

@felixboehm @DeepDiver1975 @davidjericho @cdamken @jnweiger

mention-bot · 2017-01-08T23:12:49Z

@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ogoffart, @ckamm and @dragotin to be potential reviewers.

phil-davis · 2017-01-09T05:52:38Z

src/libsync/propagateupload.h

+/**
+ * @brief The PropagateUploadBundle class is a container class for upload jobs under chunking size.
+ *
+ * It will also ensure proper bandwidth utilization vs bookkeeping balance, and that in case no other items then under chunk uploads are available,


I don't understand the detailed meaning of:
"and that in case no other items then under chunk uploads are available,"

phil-davis · 2017-01-09T06:01:38Z

I made some minor edit suggestions for comments in #5441
The principle of this seems very useful - for example I am a long way (in round-trip-time ping...) from my server and my upload speed is not so good (1 to 3 Mbps range). I often have cases where there are a few big files and a lot of small files. That often results in sub-optimal total transfer time, because a some big files might chug away for a while (3 in parallel), then it gets to a heap of small files and those then have to slowly get created on the server... If we make sure that the upload of lots of small files is running in one stream(s) in parallel with big files in another stream(s) then the things that need server time and the things that need raw transfer time can happen concurrently and the total elapsed time should be minimized.
👍 for the concept. I am not familiar enough with the overall client design to give a good opinion on the detailed implementation.

mrow4a · 2017-01-09T11:09:36Z

Ok, it works awesome, need to check it in enterprise scale. Remember, if someone want to test it, one needs a server capability / enviromental variable on the client - otherwise it will sync the old way.

    static const auto bundling = qgetenv("OWNCLOUD_BUNDLING");
    if (bundling == "0") return false;
    if (bundling == "1") return true;

    return _capabilities["dav"].toMap()["bundling"].toByteArray() >= "1.0";

@phil-davis Could you rebase and check again! I made a change to be sure that it parallelises correctly for ALL items. It was not previously, so 3 big downloads will block everything if placed in alphabetical order.

ckamm

Here's a detailed review.

From a high level point of view I think this kind of request prioritization is a good idea and while I'm not certain about all details, tweaking the parameters later looks relatively easy.

I'm worrying about all the code duplication in PropagateFiles relative to PropagateDirectory. Is there really a compelling reason for having a second level of job-container, instead of folding this into PropagateDirectory? If there is, maybe a common base class between the two would help reducing the duplication that's going on.

Apart from that, see the detail comments.

ckamm · 2017-01-10T09:50:27Z

src/libsync/capabilities.cpp

+    if (bundling == "0") return false;
+    if (bundling == "1") return true;
+
+    return _capabilities["dav"].toMap()["bundling"].toByteArray() >= "1.0";


Like this you get the typical "2.1" > "10.1" problem. Could this just be an integer?

I just reproduced what @ogoffart did with chunking, nearly copy paste from ChunkingNG

Okay, I didn't realize. That doesn't make the comparison any more correct though, even if it does set precedent for having "N.M" versions in the capabilities (which I wasn't aware of). Then I suggest we merge as-is and fix both in a follow-up.

ckamm · 2017-01-10T09:54:35Z

src/libsync/owncloudpropagator.h

+    QScopedPointer<PropagateItemJob> _firstJob;
+
+    // e.g: create class which will handle bundled uploads and bandwidth utilization vs bookkeeping balance
+    QScopedPointer<PropagatorJob> _filesJob;


It looks like this could be typed QScopedPointer<PropagateFiles> and you could avoid a bunch of casts.

ckamm · 2017-01-10T09:56:15Z

src/libsync/owncloudpropagator.cpp

+            // Ensure that only new files are inserted into PropagateFiles
+            if (enableBundledRequests && item->_instruction == CSYNC_INSTRUCTION_NEW) {
+                // Get PropagateFiles container job
+                PropagateFiles* filesJob = 0;


I'd write this as

auto& filesJob = directories.top().second->_filesJob; if (!filesJob) { filesJob = new PropagateFiles(this); }

ckamm · 2017-01-10T09:59:43Z

src/libsync/owncloudpropagator.cpp

+{
+    // A small filesize item is a file whose transfer time
+    // typically will be lower than its bookkeeping time.
+    static uint smallFileSize;


I'd prefer explicit = 0 here

Again, copy from ChunkingNG

ckamm · 2017-01-10T10:05:52Z

src/libsync/owncloudpropagator.cpp

+        if (subJobsIterator.value()->_state == Finished) {
+            // If this item is finished, remove it from _subJobs as it is not needed anymore
+            // Note that in this case remove() from QVector will just perform memmove of pointer array items.
+            PropagatorJob * job = subJobsIterator.value();


Please do PropagatorJob* job = subJobsIterator.next(); at the very beginning to clean up all the subJobsIterator.value() calls.

ckamm · 2017-01-10T10:35:24Z

src/libsync/propagateuploadbundle.cpp

+    } else {
+        // There are no remaining or pending standard jobs in the whole sync
+        // This also means that _standardJobs is empty
+        Q_ASSERT(!scheduleNextJobRoutine(_standardJobs));


This assert looks dangerous because it can have side effects.

ckamm · 2017-01-10T10:36:02Z

src/libsync/propagateuploadbundle.cpp

+        // This also means that _standardJobs is empty
+        Q_ASSERT(!scheduleNextJobRoutine(_standardJobs));
+
+        // Parallelise itself into more flows flows


"flows flows"

ckamm · 2017-01-10T10:38:29Z

src/libsync/propagateuploadbundle.cpp

+    Q_ASSERT(job);
+
+    // Reduce the global counter of db or standard jobs
+    if (job->_item->_size <= _propagator->smallFileSize()){


This decision of whether something is standard or db is reiterated in a bunch of places. Definitely make a function for it so the conditions don't go out of sync.

ckamm · 2017-01-10T10:39:27Z

src/libsync/propagateuploadbundle.cpp

+                return true;
+            } else {
+                // This container does not contain any remaining dbJobs
+                if(_runningNow > 1){


minor: A bunch of times in this function spacing is missing: "if(" and "){"

ckamm · 2017-01-10T10:42:00Z

src/libsync/owncloudpropagator.cpp

-                directories.top().second->append(current);
+        } else {
+            // Ensure that only new files are inserted into PropagateFiles
+            if (enableBundledRequests && item->_instruction == CSYNC_INSTRUCTION_NEW) {


As far as I can tell this prioritization code has nothing to do with bundling. Why is it gated by the bundling flag?

It is basis for dispatching bundles in the future, this is why. You can call this feature bundling 1.0. i can change a name however. I also give a flag, because this is now cross-folder, some people might not like it, so I give them the chance to sync folder by folder just by changing one capability on server.

mrow4a · 2017-01-10T11:58:35Z

@ckamm Because PropagateDirectory is much more complicated thinking about cases to handle, did not want to introduce even more complexity, we will get lost.

Also I wanted to make this "pluggable" so you can easily switch it on and off.

Mind that PropagateFiles is only for new files without any "special" cases, just normal upload/download, chunked or not. No _first job, delete cases, WaitInDirectory etc.

mrow4a · 2017-01-23T16:47:58Z

Hello,

Changed a little bit a structure of this and been testing it on enterprise setup, details can be found in this document @ogoffart @guruz @jturcotte @felixboehm, which is part of my CS3 presentation :
SYNC OPTIMIZATIONS - SCHEDULING
sync-optimizations-mrowczynski-scheduling.pdf

Features:

Ensured balance between jobs
Is entriely plugable, cross-folder and separate from Propagate Directory main logic
Only New and Updated files are being propagated (data transfers)
Jobs are created lazily

mrow4a · 2017-01-24T18:28:31Z

src/libsync/propagatefiles.cpp

+            finalize();
+            return true;
+        }
+        _subJobs.reserve(_totalItems);


I think I need to get rid of that, since I am wasting memory here, dont I?

mrow4a · 2017-01-24T18:30:06Z

src/libsync/propagatefiles.cpp

+bool PropagateFiles::scheduleNewJob(QVector<SyncFileItemPtr> &syncJobs){
+    // This function is used to schedule new job and lazily create job from sync items
+    Q_ASSERT(!syncJobs.isEmpty());
+    const SyncFileItemPtr &item = syncJobs.takeFirst();


We are planning to use only 5.8 dont we? Or should I still keep compatibility with Qt4? (My build is failing because this is used in Qt5)

No the plan is to use 5.6 everywhere we can, and still use Qt4 on linux platforms that we don't ship Qt (unless we manage to bundle Qt). Qt 5.6 is LTS and will be receiving patches until 2019 while 5.8 only until 2018.

…okkeeping #5391 #5390 #4498 #1633 #4454

mrow4a · 2017-01-25T20:47:05Z

Ok, checked with unit tests and they covered the code, everything is passing, please also mind that https://github.com/owncloud/client/pull/5440/files#diff-7e5082f89a138020f2b1d37fc97d17dbR319 this line has to be adjusted before merging, there is TODO

jturcotte · 2017-02-16T23:10:55Z

src/libsync/owncloudpropagator.cpp

@@ -388,6 +395,9 @@ void OwncloudPropagator::start(const SyncFileItemVector& items)
                currentDirJob->append(dir);
            }
            directories.push(qMakePair(item->destination() + "/" , dir));
+        } else if (enableScheduledRequests
+              && (item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_SYNC)) {
+            filesJob->append(item);


This changes one purpose of the PropagateDirectory structure: update the directory's etag in the database only once child files have been synced properly.

I can't wrap my head about the conditions where this could be an issue, but @ogoffart should know if this could cause concrete problems.

See e.g.

client/src/libsync/owncloudpropagator.cpp

Line 705 in 1cec2ca

// For new directories we always want to update the etag once

Thanks for pointing out, did not know about this, dont think it is difficult to resolve but let @ogoffart see this

@mrow4a @jturcotte @ogoffart If I'm not mistaken the following is a situation in which that dependency structure could lead to an abort-related problem:

Initial state in db and server: / - "etag/1" /A - "etagA1" /A/F - "etagF1"

now when someone touches /A/F on the server, the server state becomes

Server state, after touching /A/F / - "etag/2" /A - "etagA2" /A/F - "etagF2"

but if I understand correctly the propagation dependency graph is

PropagateDirectory (/) |- PropagateDirectory (/A) |- PropagateFiles |- PropagateItem (/A/F)

meaning that /A and /A/F run independently of each other. So it would be possible to completely finish propagating /A before the file transfer /A/F is done. Then aborting the sync run could lead to the db tree

/ - "etag/1" /A - "etagA2" /A/F - "etagF1"

Which means a follow up sync would probably not pick up on /A/F being out of date.

Maybe this hints at a second dependency problem: Currently local and remote MkDir are FullParallelism - so isn't there a chance that with this change one could run into a case where the client wants to download or upload into a non-existant directory?

Possibly I'm missing something, feel free to point out incorrectness.

No it wont upload to non-existent directory, because directory structure / resolving conflicts etc are done before any transfers, and directory deletions are done after https://github.com/owncloud/client/pull/5440/files#diff-c9731f430e8a29b13deaaf73bc4a4e22R56
https://github.com/owncloud/client/pull/5440/files#diff-20b960bb10cf3c0781a80fb3e5775241R493
https://github.com/owncloud/client/pull/5440/files#diff-7e5082f89a138020f2b1d37fc97d17dbR420

About the etag propagation did not look into the problem yet, because we dont have yet smashbox automation for PRs (it is nearly done) and I am busy on the server side before the release.

@mrow4a Okay, I didn't see that. (btw, _runningNow is gone, so this branch doesn't compile - probably was rebased at some point?) In that case the second problem can indeed not happen.

mrow4a · 2017-03-11T10:17:15Z

For LAN sync - https://central.owncloud.org/t/gsoc-2017-fast-lan-sync/6271 - we need this PR. Sync clients need to be "on the same page", and be finished with any metadata logic around, so that they can "safely" transfer the data, as in this PR. LAN Sync has to be done only on raw PUT/GETs and this logic has to be abstracted from "Directory sync logic" (basicaly what this PR is doing). @hodyroff @DeepDiver1975

guruz · 2017-03-23T15:52:07Z

@mrow4a: make sure to try to interrupt syncs in the middle and then resume them (also multiple times) and check if this will really still lead to correctly synchronized directories and not to deletes file or files that are not synced.
@mrow4a: please make sure smashbox reliability sync tests pass
@mrow4a: please make sure S3 syncs faster than without the patch

ogoffart · 2017-04-28T10:19:40Z

I removed the milestone because this would still need more work, and I don't think it is worth it at all. But I leave it open for the sake of discussion.

ogoffart · 2019-10-30T13:31:52Z

Closing outdated pull request

mrow4a added Enhancement Performance labels Jan 8, 2017

mrow4a self-assigned this Jan 8, 2017

mrow4a requested review from ckamm, jturcotte, ogoffart and guruz January 8, 2017 23:12

mrow4a changed the title ~~Group requests to ensure balance between bandwidth utilization and bookkeeping~~ Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 Jan 8, 2017

mrow4a force-pushed the group_schedule branch from e85499d to f5298b5 Compare January 9, 2017 00:06

phil-davis reviewed Jan 9, 2017

View reviewed changes

mrow4a force-pushed the group_schedule branch 2 times, most recently from 262e800 to 3701299 Compare January 9, 2017 11:07

mrow4a mentioned this pull request Jan 9, 2017

Delete finished job in the PropagateDirectory scheduleNextJob. #5269 #5400

Merged

ckamm suggested changes Jan 10, 2017

View reviewed changes

mrow4a mentioned this pull request Jan 12, 2017

Add sync files scheduler class #5269

Closed

mrow4a force-pushed the group_schedule branch from fa5eb85 to 0e58dd7 Compare January 23, 2017 16:40

mrow4a commented Jan 24, 2017

View reviewed changes

mrow4a force-pushed the group_schedule branch from 0e58dd7 to 83e38c5 Compare January 25, 2017 13:17

guruz added this to the 2.4.0 milestone Jan 25, 2017

Group requests to ensure balance between bandwidth utilization and bo…

b6af59d

…okkeeping #5391 #5390 #4498 #1633 #4454

mrow4a force-pushed the group_schedule branch from 83e38c5 to b6af59d Compare January 25, 2017 20:36

Merge branch 'master' into group_schedule

98e6eba

jturcotte reviewed Feb 16, 2017

View reviewed changes

mrow4a mentioned this pull request Feb 17, 2017

Discussion - Separate sync specific jobs from data transfer jobs #5406

Closed

guruz mentioned this pull request Mar 23, 2017

Prototype: Folder items scheduler - evaluation attribute sorted tree #5349

Closed

ogoffart removed this from the 2.4.0 milestone Apr 28, 2017

guruz added the DO NOT MERGE YET label May 24, 2018

ogoffart closed this Oct 30, 2019

TheOneRing deleted the group_schedule branch December 2, 2019 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 #5440

Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 #5440

mrow4a commented Jan 8, 2017 •

edited

Loading

mention-bot commented Jan 8, 2017

phil-davis Jan 9, 2017

phil-davis commented Jan 9, 2017

mrow4a commented Jan 9, 2017 •

edited

Loading

ckamm left a comment

ckamm Jan 10, 2017

mrow4a Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

mrow4a Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

ckamm Jan 10, 2017

mrow4a Jan 10, 2017

mrow4a commented Jan 10, 2017

mrow4a commented Jan 23, 2017

mrow4a Jan 24, 2017

mrow4a Jan 24, 2017 •

edited

Loading

jturcotte Jan 25, 2017

mrow4a commented Jan 25, 2017

jturcotte Feb 16, 2017

mrow4a Feb 16, 2017

ckamm Mar 24, 2017

mrow4a Mar 24, 2017

ckamm Mar 28, 2017 •

edited

Loading

mrow4a commented Mar 11, 2017

guruz commented Mar 23, 2017

ogoffart commented Apr 28, 2017

ogoffart commented Oct 30, 2019

Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 #5440

Group requests to ensure balance between bandwidth utilization and bookkeeping #5391 #5390 #4498 #1633 #4454 #5440

Conversation

mrow4a commented Jan 8, 2017 • edited Loading

Idea

Checkpoints

Details

mention-bot commented Jan 8, 2017

Choose a reason for hiding this comment

phil-davis commented Jan 9, 2017

mrow4a commented Jan 9, 2017 • edited Loading

ckamm left a 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

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 Jan 10, 2017

mrow4a commented Jan 23, 2017

Choose a reason for hiding this comment

mrow4a Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrow4a commented Jan 25, 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

ckamm Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

mrow4a commented Mar 11, 2017

guruz commented Mar 23, 2017

ogoffart commented Apr 28, 2017

ogoffart commented Oct 30, 2019

mrow4a commented Jan 8, 2017 •

edited

Loading

mrow4a commented Jan 9, 2017 •

edited

Loading

mrow4a Jan 24, 2017 •

edited

Loading

ckamm Mar 28, 2017 •

edited

Loading