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

mrow4a · 2016-12-01T18:57:24Z

Case Scenario 1:
Carlos and his Zombies just went back from holidays, he opens the computer and tries to sync all his photos - 4GB. With his link, to upload all the photos will take around 1 hour. He starts working since next day is monday, and he puts in the various folders his new files.

Case Scenario 2:
Carlos didnt use sync client for longer time, he want to do the initial sync of his files. He starts sync of a lot of folders with a lot of files. Total sync time will be around 30 minutes. Within all these files, there are some folders on which he collaborates on the document. He want to share the document he is editing during the sync, and this folder is not big (it contains only documents).

EXPECTATIONS:
In the very nested structure, there is a folder with only small changes, and folder with a lot of new files, which sync time will be very long. It is expected, that the folders with small changes will be synced first. If I pause-start sync, my small file schould by synced first

REALITY:
OC client currently is "I just found folder on filesystem and I will try to sync its contents to the cloud" structured. This means, that if you place a small change in some nexted folder tree, and before it there is another nested tree with a lot of files, your document will wait till sync comes to your folder

Solution:
Effect 1:
I removed the caching of the Finished Jobs, and I just remove them from the stack. I am using also Iterator instead of for loop.

Effect 2:
Oc sync is structurising the sync in the following manner, not regarding on the size of the folders:

THE TREES BELOW ARE TREES OF CHANGES - These folders could contain a lot of unchanged files in each. Tree is being build only basing on size of changes within specific subtree.

Important note: all file operations(IGNORE, MOVE, ETC) except for PUT and GET, are assigned with HighPriority, it means they will be synced with insertion order (the same as previously). PUT and GET predicates are based on their file sizes and are assigned NormalPriority. Folders (subtrees) are assigned ContainerPriority, which means their predicate is size of the changes within this subtree. REMOVED folders are HighPriority and will be synced old way.

ownCloud Root
├── less-important-folder-10MB/
       ├── nested-folderes-files 9MB
       ├── less-important-file 1MB
       └── nested-folderes-files 4MB
├── Home&Work-10GB/
        ├── folder with nested 10GB of photos to be sync
        ├── very-very-important folder with single documents to sync - 2MB
        └── less-important-document - 1MB
├── important-folder-12MB/
└── less-important-folder - 5MB/
        └── important-file-1MB

However, this algorithm (not using loop for all the items, only for folders within folder - complexity 0(n) where n is number of folders in the folder) builds it as follows, looking on size predicate for files and size of items within folders:

ownCloud new Execution Tree:
├── less-important-folder-5MB/
       └── important-file-1MB
├── less-important-folder-10MB/
       ├── less-important-file 1MB
       ├── nested-folderes-files 4MB
       └── nested-folderes-files 9MB
├── important-folder-12MB/
├── Home&Work-10GB/
        ├── less-important-document - 1MB
        ├── very-very-important folder with single documents to sync - 2MB
        └── folder with nested 10GB of photos to be sync

I have tested it on similar structure and it works really nice :> @ogoffart @DeepDiver1975 @guruz @felixboehm

mention-bot · 2016-12-01T18:57:26Z

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

mrow4a · 2016-12-01T19:02:05Z

This algorithm live sorts the items withing all the folders according to its size or the total size of the contained items. It CREATES ON FLY a specific sorted tree, while ADDING items to the folder using specific data structure QMap with key being predicate and value being Job (not using sorting loop! this will be inefficient for a lot of files, it will loop only for folders, which number is not expected to be exceeding milion I guess).

Quick reminder, QMap is characterised that it sorts its item by the key. If the key is the same, e.g. 0, items are sorted in the insertion order.

mrow4a · 2016-12-01T19:14:53Z

This should also solve #1633 since this will By The Way balance upload/download by placing the files/folders of the same sizes close to each other. Our 3 separate threads should create nice pipe utilization in this case.

Please close the issue after merge

mrow4a · 2016-12-03T12:20:26Z

src/libsync/owncloudpropagator.cpp

+            // TODO: The call for getJobPredicateValue should recursively traverse all kids in _containerJobs of _containerjobs,
+            // in order to update their predicates. Root job should call updatePredicates in owncloudPropagator as a last step before run
+            // , and save flag _folderPredicateUpdated = true
+             _subJobs.insertMulti(job->getJobPredicateValue(), job);


This one should basicaly be moved to the moment before root get started, and the function _root->updateContainersPredicates() should perform left-deep traversal of all the folders with changed files ( _containerJobs), and be added to _subJobs list with getJobPredicateValue(), job>. This has to be done like that, because files are being added to containers and we have to update _subJobs list of all folders starting from deepest ones.

mrow4a · 2016-12-04T18:47:39Z

src/libsync/owncloudpropagator.h

+
+    // This uses recursion to perform Depth-First Traversal of the directories with changes trees
+    // If the given (this) directory contains _containerJobs, it will call updateJob on that child dir job, otherwise does nothing
+    void updateJobPredicateValues();


This is initiated (run first) for _root, after all jobs are appended. It will execute Post-Order Depth-First Tree Traversal in order to visit directories kids (_containerJobs) of given _root, starting from bottom, and visiting their respecive directories kids (_containerJobs).

phil-davis · 2016-12-05T05:39:18Z

See #5352 to fix a few comment typos.

guruz · 2016-12-05T14:08:28Z

src/libsync/owncloudpropagator.h

    }

    void append(PropagatorJob *subJob) {
-        _subJobs.append(subJob);
+        _subJobsPriority += subJob->getJobPredicateValue();


I see getJobPredicateValue is called here.. but there is also updateJobPredicateValues... does this not cause too much load? Maybe I misunderstand..

These are two different functions. getJobPredicateValue will just get the present value of the sync item (ignore file etc, sync file (file size)) and for directories, we insert them to _containerJobs.

updateJobPredicateValue is recursively traversing only _containerJobs tree, thus only directories with changes. This one will be called once for the _root job, and recursively visit all "changed" directories. _subJobs are untouched, and I will not touch that, because this could contain thousands of items, while _containerJobs will never be deep.

Maybe more commenting..

@guruz Ok, on the weekend will write algorithm documentation for that in https://github.com/owncloud/documentation/tree/master/developer_manual

guruz · 2016-12-05T14:13:37Z

src/libsync/owncloudpropagator.h

+        if(subJob->_priority == JobPriority::ContainerItemsPriority){
+            _containerJobs.append(subJob);
+        } else {
+            _subJobs.insertMulti(subJob->getJobPredicateValue(), subJob);


To make the intent clear, change QMap to QMultiMap?

Ohh, didnt know about this one. Nice

guruz · 2016-12-05T14:14:06Z

src/libsync/owncloudpropagator.cpp


-    for (int i = _firstUnfinishedSubJob; i < subJobsCount; ++i) {
-        if (_subJobs.at(i)->_state == Finished) {
+    QMutableMapIterator<quint64, PropagatorJob *> subJobsIterator(_subJobs);


Please add a comment that this iterates by predicate and also iterated over the multiple values of a multi map

Isnt it just iterates over the items it has in the sorting order? I thought it is self explaining.

For me it was not...

guruz · 2016-12-05T14:17:13Z

src/libsync/owncloudpropagator.cpp

@@ -396,9 +396,12 @@ void OwncloudPropagator::start(const SyncFileItemVector& items)
    }


FYI I added you on several issues here in github, please mention them in the commit message if you think they are handled by this

guruz · 2016-12-05T14:18:04Z

src/libsync/owncloudpropagator.h

-    // all the sub files or sub directories.
-    QVector<PropagatorJob *> _subJobs;
+    // all the sub files or sub directories. This map has to be updated with _containerJobs
+    // QMap is ordered by the key value, or in case of equal keys (e.g 0) by insertion order.


Please clarify what the key value actually is here in the comment (it's the predicate value aka file sizes right?)

guruz · 2016-12-05T14:18:36Z

src/libsync/owncloudpropagator.h

+    // QMap is ordered by the key value, or in case of equal keys (e.g 0) by insertion order.
+    QMap<quint64, PropagatorJob *> _subJobs;
+
+    QVector<PropagatorJob *> _containerJobs;


Please add detailed comment of the destinction between a container job and a sub job.

guruz

Thanks, interesting..
Needs more comments.. :)
What does smashbox say?

mrow4a · 2016-12-05T16:27:56Z

@guruz Whom to contact for smashbox tests? How do you do that normally?

@owncloud/qa do you automatize smashbox tests also for client?

EDIT: submitted corrections

mrow4a · 2016-12-05T16:31:06Z

Please after acceptance, close #4498 (comment)
and also #5077 (comment)

guruz · 2016-12-07T10:30:07Z

Please after acceptance, close

@mrow4a That's why I wrote "please mention them in the commit message if you think they are handled by this".. then they are linked.

mrow4a · 2016-12-07T16:17:17Z

@guruz Ok, I misunderstood you, though you mean just comment here. Will correct it.

mrow4a · 2016-12-08T13:10:08Z

Ohh, so I should just run it localy, I though you had some QA server on which you test your branches. Ok, will test it localy agains the tests from ownCloud CI

jturcotte

My list of changes I'd like to see after the discussion today:

Rename "Predicate" to something like "Function"
Set the sort function of a folder vs. its sibblings to be the most recent modification time of all its children

mrow4a · 2016-12-15T10:04:41Z

src/libsync/owncloudpropagator.cpp


-    // FIXME!  we should probably cache this result
+    //this will get the _container jobs within this parent directory and iterate over them
+    QMutableVectorIterator<PropagatorJob *> containerJobsIterator(_containerJobs);


Should I use List? @ogoffart because of the remove?

mrow4a · 2016-12-15T10:12:44Z

src/libsync/propagateremotedelete.h

@@ -48,7 +48,7 @@ class PropagateRemoteDelete : public PropagateItemJob {
    QPointer<DeleteJob> _job;
 public:
    PropagateRemoteDelete (OwncloudPropagator* propagator,const SyncFileItemPtr& item)
-        : PropagateItemJob(propagator, item) {}
+        : PropagateItemJob(propagator, item, JobPriority::LastOutPriority) {}


mrow4a · 2016-12-15T10:13:01Z

src/libsync/propagatorjobs.h

@@ -40,7 +40,8 @@ static const char checkSumAdlerC[] = "Adler32";
 class PropagateLocalRemove : public PropagateItemJob {
    Q_OBJECT
 public:
-    PropagateLocalRemove (OwncloudPropagator* propagator,const SyncFileItemPtr& item)  : PropagateItemJob(propagator, item) {}
+    PropagateLocalRemove (OwncloudPropagator* propagator,const SyncFileItemPtr& item)
+        : PropagateItemJob(propagator, item, JobPriority::FirstOutPriority) {}


felixboehm · 2016-12-19T19:47:53Z

@mrow4a Please add unit tests, you are rising complexity without testing.
Please check performance impact with large amount of files.

ogoffart · 2017-01-11T14:19:36Z

We have not established yet best way to schedule the file.
I think the priority goes to get bundling in.
And this issue should not block 2.3.0

mrow4a · 2017-01-11T14:53:08Z

Yes, I think this is yet another feature, independent from other things, but worth consideration.

guruz · 2017-03-23T15:51:21Z

AFAIK this only makes sense if #5440 is done?

ogoffart · 2017-04-28T08:42:17Z

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.

mrow4a · 2017-04-28T08:51:14Z

I still think that sorting by modification time and syncing that way might be a good idea.

guruz · 2017-05-03T10:05:30Z

and I don't think it is worth it at all.

I think it might be.

So let's see after the tests/benchmarks about this.
No rush now for 2.4 or 3.0..

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

Closing outdated pull request

mrow4a changed the title ~~Add folder items scheduler with specific predicate - item sizes ascending~~ Folder items scheduler with specific predicate - item sizes ascending Dec 1, 2016

mrow4a force-pushed the scheduler branch 4 times, most recently from 56afdbf to 786478a Compare December 1, 2016 21:03

mrow4a commented Dec 3, 2016

View reviewed changes

mrow4a force-pushed the scheduler branch 2 times, most recently from d319e86 to d252113 Compare December 4, 2016 18:45

mrow4a commented Dec 4, 2016

View reviewed changes

guruz assigned ogoffart Dec 5, 2016

guruz added this to the 2.3.0 milestone Dec 5, 2016

guruz added the Performance label Dec 5, 2016

guruz reviewed Dec 5, 2016

View reviewed changes

guruz suggested changes Dec 5, 2016

View reviewed changes

Add folder items scheduler by specific predicate - item sizes ascending

353cd16

mrow4a force-pushed the scheduler branch from ee09217 to 353cd16 Compare December 5, 2016 16:56

mrow4a mentioned this pull request Dec 8, 2016

Prototype: Requests bundling feature implementation #5319

Closed

10 tasks

jturcotte suggested changes Dec 13, 2016

View reviewed changes

mrow4a changed the title ~~Folder items scheduler with specific predicate - item sizes ascending~~ Folder items scheduler by evaluation attribute Dec 14, 2016

mrow4a commented Dec 15, 2016

View reviewed changes

add scheduling by modification time

6a8e718

mrow4a force-pushed the scheduler branch from 40dccca to 6a8e718 Compare December 15, 2016 14:25

mrow4a changed the title ~~Folder items scheduler by evaluation attribute~~ Folder items scheduler - evaluation attribute sorted tree Dec 18, 2016

mrow4a mentioned this pull request Dec 18, 2016

In the mixed file size scenario bandwidth is very underutilized - enterprise setup #5391

Open

mrow4a changed the title ~~Folder items scheduler - evaluation attribute sorted tree~~ Prototype: Folder items scheduler - evaluation attribute sorted tree Dec 20, 2016

mrow4a mentioned this pull request Dec 21, 2016

Download small files first? #5410

Closed

mrow4a mentioned this pull request Jan 8, 2017

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

Closed

4 tasks

ogoffart modified the milestones: 3.0.0, 2.3.0 Jan 11, 2017

guruz modified the milestones: 2.4.0, 3.0.0 Mar 16, 2017

ogoffart removed this from the 2.4.0 milestone Apr 28, 2017

ogoffart removed their assignment Apr 28, 2017

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

ogoffart closed this Oct 30, 2019

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

		@@ -396,9 +396,12 @@ void OwncloudPropagator::start(const SyncFileItemVector& items)
		}

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

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

Conversation

mrow4a commented Dec 1, 2016 • edited Loading

mention-bot commented Dec 1, 2016

mrow4a commented Dec 1, 2016 • edited Loading

mrow4a commented Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

mrow4a Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

phil-davis commented Dec 5, 2016

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

guruz left a comment

Choose a reason for hiding this comment

mrow4a commented Dec 5, 2016 • edited Loading

mrow4a commented Dec 5, 2016 • edited Loading

guruz commented Dec 7, 2016

mrow4a commented Dec 7, 2016

mrow4a commented Dec 8, 2016

jturcotte left a comment • edited by mrow4a Loading

Choose a reason for hiding this comment

mrow4a Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixboehm commented Dec 19, 2016

ogoffart commented Jan 11, 2017

mrow4a commented Jan 11, 2017

guruz commented Mar 23, 2017

ogoffart commented Apr 28, 2017

mrow4a commented Apr 28, 2017

guruz commented May 3, 2017

ogoffart commented Oct 30, 2019

mrow4a commented Dec 1, 2016 •

edited

Loading

mrow4a commented Dec 1, 2016 •

edited

Loading

mrow4a commented Dec 1, 2016 •

edited

Loading

mrow4a Dec 4, 2016 •

edited

Loading

mrow4a commented Dec 5, 2016 •

edited

Loading

mrow4a commented Dec 5, 2016 •

edited

Loading

jturcotte left a comment •

edited by mrow4a

Loading

mrow4a Dec 15, 2016 •

edited

Loading