Optimization for very slow processing after downloading folder content #5150

JorisBodin · 2020-01-10T14:56:57Z

Hello,

Fixes #5086

My tests were performed with a folder of 1044 files and Galaxy S8

Benchmark
On master:
Insert (first download):
50 seconds.

Update (refresh):
20 seconds

With this pull request:
Insert:
20 seconds

Update:
6 seconds

There are still 2 things to improve
First, the doubleCheck in FileContentProvider:

This check is here because saveFolder (in FileDataStorageManager) is sometimes called twice for an insert.
I've found two cases where that happens, one of which is simple to fix.
The first time the application is started, the WRITE_EXTERNAL_STORAGE permission is requested. When the user accepts, syncAndUpdateFolder is called in onResume and in onRequestPermissionsResult.
Delete syncAndUpdateFolder in onRequestPermissionsResult is solution.

Also the first time the application is started, if refreshing the folder, syncAndUpdateFolder is called twice

The solution would be to block the simultaneous call of saveFolder.

Benchmark
If doubleCheck is removed:
Insert (first download):
13 seconds.

Update (refresh):
6 seconds

The other thing to improve is this check #4647
(It is discussed in #5086)
I haven't found an optimization solution for this case. 😞

Benchmark
But if we remove it
Insert (first download):
1 seconds.

Update (refresh):
5 seconds

I also took the time to refactor the FileDataStorageManager class, I have removed the inconsistencies and duplications that have accumulated since the beginning. 2298 lines to 1887 lines
It allowed me to better understand the code and better optimize it.

Next week, we're thinking of coming to hackweek in Berlin. Will you be there?

Signed-off-by: Joris Bodin <[email protected]>

avg 15s to 0.2s for 900 new files ! Remove unnecessary double check and very old method not used (since 2015) Signed-off-by: Joris Bodin <[email protected]>

Signed-off-by: Joris Bodin <[email protected]>

Before avg 6s (for avg new 1000 files) now 0.2s with applyBatch Signed-off-by: Joris Bodin <[email protected]>

Signed-off-by: Joris Bodin <[email protected]>

And readd localFile = mStorageManager.getFileByPath(updatedFile.getRemotePath()); And optimize insert virtualFile Signed-off-by: Joris Bodin <[email protected]>

Rename file to ocFile for OCFile object to avoid ambiguity with the objects of type File Remove unUsed variable != null (i check method caller) Add != null is necessary Rename to camelCase Consistency in the code Signed-off-by: Joris Bodin <[email protected]>

Signed-off-by: Joris Bodin <[email protected]>

codecov · 2020-01-10T18:44:29Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2a01aca). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #5150   +/-   ##
=========================================
  Coverage          ?   18.71%           
  Complexity        ?        3           
=========================================
  Files             ?      392           
  Lines             ?    34273           
  Branches          ?     5011           
=========================================
  Hits              ?     6414           
  Misses            ?    26863           
  Partials          ?      996

nextcloud-android-bot · 2020-01-15T12:01:48Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 15
- Added 28
           

Complexity decreasing per file
==============================
+ src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java  -3

See the complete overview on Codacy

nextcloud-android-bot · 2020-01-15T12:02:15Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+                            ProviderTableMeta.FILE_CONTENT_TYPE + " != 'DIR' AND " +
+                            ProviderTableMeta.FILE_ACCOUNT_OWNER + AND +
+                            ProviderTableMeta.FILE_PATH + " LIKE ?";
+                    selectionArgs = new String[]{account.name, parentPath + "%"};


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:16Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java


-            List<OCFile> files = getFolderContent(folder, false);
+                ArrayList<String> selectionArgsList = new ArrayList<>();


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:28Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+                ArrayList<String> selectionArgsList = new ArrayList<>();
+                selectionArgsList.add(account.name);
+                selectionArgsList.addAll(listPathString);
+                String[] selectionArgs = selectionArgsList.toArray(new String[0]);


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:29Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+            if (cursor != null) {
+                if (cursor.moveToFirst()) {
+                    do {
+                        OCFile ocFile = new OCFile(cursor.getString(cursor.getColumnIndex(ProviderTableMeta.FILE_PATH)));


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:30Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

                } else {
-                    file = null;
+                    ocFile = null;


Issue found: Assigning an Object to null is a code smell. Consider refactoring.

nextcloud-android-bot · 2020-01-15T12:02:32Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

-            saveFile(newFile);
-        } else {
-            throw new IllegalArgumentException("Saving a new file in an unexisting folder");
+    private boolean isFileExists(ArrayList<OCFile> filesExists, OCFile ocFile) {


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-15T12:02:33Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+        return contentProviderResults;
+    }
+
+    private ContentProviderResult[] applyBatch(ArrayList<ContentProviderOperation> operations) {


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-15T12:02:35Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

-                        }
+                        where = ProviderTableMeta.FILE_ACCOUNT_OWNER + AND +
+                            ProviderTableMeta.FILE_PATH + " = ?";
+                        selectionArgs = new String[]{account.name, parentPath};


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:36Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

     *
     * @param folder
     * @param updatedFiles
     * @param filesToRemove
     */
-    public void saveFolder(OCFile folder, Collection<OCFile> updatedFiles, Collection<OCFile> filesToRemove) {
+    public void saveFolder(OCFile folder, ArrayList<OCFile> updatedFiles, Collection<OCFile> filesToRemove) {


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-15T12:02:37Z

src/main/java/com/owncloud/android/providers/FileContentProvider.java

+                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && operation.isInsert()) {
+                    ContentValues contentValues = operation.resolveValueBackReferences(results, i);
+                    Uri newUri = insert(database, operation.getUri(), contentValues);
+                    results[i] = new ContentProviderResult(newUri);


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:38Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+
+            ArrayList<String> listPathString = new ArrayList<>();
+
+            int totalSize, processSize, loopSize;


Issue found: Use one line for each declaration, it enhances code readability.

nextcloud-android-bot · 2020-01-15T12:02:39Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

-                        if (new File(path).delete() && MimeTypeUtil.isMedia(file.getMimeType())) {
+                    if (ocFile.isDown()) {
+                        String path = ocFile.getStoragePath();
+                        if (new File(path).delete() && MimeTypeUtil.isMedia(ocFile.getMimeType())) {


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:41Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+                loopSize = Math.min((totalSize - processSize), 998);
+
+                listPathString.clear();
+                StringBuilder inList = new StringBuilder(folderContent.size() * 2);


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:42Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+        return deleted;
+    }
+
+    private int deleteFiles(Uri uri, String where, String[] whereArgs) {


Issue found: Consider using varargs for methods or constructors which take an array the last parameter.

nextcloud-android-bot · 2020-01-15T12:02:44Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+                    if (i > 0) {
+                        inList.append(",");
+                    }
+                    inList.append("?");


Issue found: Avoid appending characters as strings in StringBuffer.append.

nextcloud-android-bot · 2020-01-15T12:02:46Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+        ContentValues contentValues;
+        // prepare operations to insert or update files to save in the given folder
+        for (OCShare share : shares) {
+            contentValues = new ContentValues();


Issue found: Avoid instantiating new objects inside loops

nextcloud-android-bot · 2020-01-15T12:02:47Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

+        return executeQuery(uri, null, selection, selectionArgs, null, errorMessage);
+    }
+
+    private ContentProviderResult[] applyBatch(ArrayList<ContentProviderOperation> operations, String errorMessage) {


Issue found: Avoid using implementation types like 'ArrayList'; use the interface instead

nextcloud-android-bot · 2020-01-15T13:00:58Z

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12302.apk

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

nextcloud-android-bot · 2020-01-15T13:10:34Z

Codacy

340

Lint

Type	master	PR
Warnings	76	76
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	69
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	113
Security Warnings	44
Dodgy code Warnings	138
Total	417

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	71
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	107
Security Warnings	44
Dodgy code Warnings	138
Total	413

SpotBugs increased!

tobiasKaminsky · 2020-01-21T13:46:13Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

-    private static final String AND = "=? AND ";
-    private static final String FAILED_TO_INSERT_MSG = "Fail to insert insert file to database ";
+    private static final String AND = " = ? AND ";
+    private static final String FAILED_TO_UPDATE_MSG = "Fail to update file to database ";


It seems that it same String is used for insert and update.
Changing this to "update" is as wrong as using only "insert".

So either, we use both and change it accordingly, or we generify the error message and us "failed to insert/update file to…"

With my refactor, I've removed a lot of code duplication. So FAILED_TO_INSERT_MSG was used more than once.
However, for update, it is used in 2 places. So I changed that message to this.
We can use "insert/update", but it will be less exact.
"Fail to insert" is used in the insertFile method, if you want to merge this :D

Signed-off-by: tobiasKaminsky <[email protected]>

remove non needed exceptions Split off of #5150 Update translations Update Nextcloud Android library

JorisBodin added 25 commits December 24, 2019 11:20

Refactor fileExists

2ff6d73

Signed-off-by: Joris Bodin <[email protected]>

Optimization isFileExists

aa11e9b

Signed-off-by: Joris Bodin <[email protected]>

Fix sql injection

068ff22

Signed-off-by: Joris Bodin <[email protected]>

bulkInsert is more efficient

339770f

avg 15s to 0.2s for 900 new files ! Remove unnecessary double check and very old method not used (since 2015) Signed-off-by: Joris Bodin <[email protected]>

Split query for SQL variables limit and remove unused method

87a8ab5

Signed-off-by: Joris Bodin <[email protected]>

Remove bulkInsert and optimize applyBatch (only >= Android M)

1c19846

Before avg 6s (for avg new 1000 files) now 0.2s with applyBatch Signed-off-by: Joris Bodin <[email protected]>

Optimization RemoveSharesInFolder

235ca28

Signed-off-by: Joris Bodin <[email protected]>

Begin refactor FileDataStorageManager

724147d

And readd localFile = mStorageManager.getFileByPath(updatedFile.getRemotePath()); And optimize insert virtualFile Signed-off-by: Joris Bodin <[email protected]>

Merge remote-tracking branch 'origin/master' into optimization

af60660

Extract method for applyBatch and updateFiles

2b69892

Signed-off-by: Joris Bodin <[email protected]>

Extract insertFile and DeleteFiles

f7c8bf2

Signed-off-by: Joris Bodin <[email protected]>

Finish extract deleteFiles and applyBatch

65acb83

Signed-off-by: Joris Bodin <[email protected]>

Merge remote-tracking branch 'origin/master' into optimization

56f8b3b

Prepare remove duplication

f303cd0

Signed-off-by: Joris Bodin <[email protected]>

Add old comment

f47fa94

Signed-off-by: Joris Bodin <[email protected]>

Remove createContentValueForFile doublon

d7122e7

Signed-off-by: Joris Bodin <[email protected]>

Export createContentValue for saveFile()

0d61142

Signed-off-by: Joris Bodin <[email protected]>

Merge branch 'master' into optimization

b17373c

Fix when search file

0960d08

Signed-off-by: Joris Bodin <[email protected]>

Merge branch 'master' into optimization

bff8068

Fix bug on first connect

4ce14ab

Signed-off-by: Joris Bodin <[email protected]>

Merge branch 'master' into optimization

0f54b71

Oups

87855c1

Signed-off-by: Joris Bodin <[email protected]>

Merge branch 'master' into optimization

2a01aca

Merge branch 'master' into optimization

61fe75e

nextcloud-android-bot reviewed Jan 15, 2020

View reviewed changes

JorisBodin mentioned this pull request Jan 15, 2020

Optimization step 2 - when doubleCheck is removed #5208

Open

AndyScherzinger added the 3. to review label Jan 15, 2020

JorisBodin mentioned this pull request Jan 15, 2020

Optimization step 3 - Fix vanishing images #5210

Open

tobiasKaminsky reviewed Jan 21, 2020

View reviewed changes

This was referenced Jan 28, 2020

Fds tests #5336

Merged

Optimization after split up #5337

Open

tobiasKaminsky pushed a commit that referenced this pull request May 27, 2020

Split off of #5150

1fe8164

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky pushed a commit that referenced this pull request Jun 5, 2020

Split off of #5150

579d98c

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky pushed a commit that referenced this pull request Jun 5, 2020

Split off of #5150

c7998aa

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky pushed a commit that referenced this pull request Jun 10, 2020

Split off of #5150

008f1af

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky pushed a commit that referenced this pull request Jun 16, 2020

Split off of #5150

e9f1c79

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky pushed a commit that referenced this pull request Jun 16, 2020

Split off of #5150

37bf0dd

Signed-off-by: tobiasKaminsky <[email protected]>

tobiasKaminsky added a commit that referenced this pull request Jun 17, 2020

daily dev 20200617

fbb3a33

remove non needed exceptions Split off of #5150 Update translations Update Nextcloud Android library

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimization for very slow processing after downloading folder content #5150

Optimization for very slow processing after downloading folder content #5150

JorisBodin commented Jan 10, 2020 •

edited by joshtrichards

Loading

codecov bot commented Jan 10, 2020 •

edited

Loading

nextcloud-android-bot commented Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot Jan 15, 2020

nextcloud-android-bot commented Jan 15, 2020

nextcloud-android-bot commented Jan 15, 2020

tobiasKaminsky Jan 21, 2020

JorisBodin Jan 21, 2020


		List<OCFile> files = getFolderContent(folder, false);
		ArrayList<String> selectionArgsList = new ArrayList<>();


		ArrayList<String> listPathString = new ArrayList<>();

		int totalSize, processSize, loopSize;

Optimization for very slow processing after downloading folder content #5150

Are you sure you want to change the base?

Optimization for very slow processing after downloading folder content #5150

Conversation

JorisBodin commented Jan 10, 2020 • edited by joshtrichards Loading

codecov bot commented Jan 10, 2020 • edited Loading

Codecov Report

nextcloud-android-bot commented Jan 15, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-android-bot commented Jan 15, 2020

nextcloud-android-bot commented Jan 15, 2020

Codacy

Lint

SpotBugs (new)

SpotBugs (master)

SpotBugs increased!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorisBodin commented Jan 10, 2020 •

edited by joshtrichards

Loading

codecov bot commented Jan 10, 2020 •

edited

Loading