Skip check for existing files when reading file data from local DB

[starypatyk]

As discussed in #10783 and #11181.

It seems that the code to check for existing files when reading file data from the local DB is no longer needed.

In my "standard" benchmark (~330 files in a directory on a low-powered device), with this change the average execution time of the OCFileListFragment::listDirectory call is reduced from ~320 ms to ~280 ms (but the timings are not very consistent).

I have not noticed any negative side effects, but to be honest, I have no idea how to really test it.

[github-actions]

blue-Light-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11227-Screenshot-blue-Light-21-35

[starypatyk]

One of the test failures seems to be directly related to the changes:
com.owncloud.android.FileIT - testRenameFolder

I have tried to repeat the issue in the app (renaming folder manually), but this works fine for me.

I am not sure yet, if the test assumes too much or if the storagePath should be set somewhere to compensate for the removed code.

Other failures in EndToEndRandomIT seem unrelated - at least I do not see any connection.

On the other hand the failure in DialogFragmentIT might be somehow related. It might be caused by a change of the timing between test execution in DialogFragmentIT::testAccountChooserDialog and delayed execution in the FileDisplayActivity::startSyncFolderOperation.

[starypatyk]

It turned out that the file.exists check is still required for directories.

When a folder is synced locally storagePath is set for files, but not for directories.

With the original change the On Device pane stopped showing folders. 👎

I had to partially revert the previous commit and leave the check for directories. I hope this will also fix the failing testRenameFolder test.

Assuming that in the typical situation there are much more files than folders, this still gives similar performance improvement as the first attempt.

BTW - I have found one reason why I get inconsistent timings. At some point after starting the app, the JIT compiler decides to optimize the FileDao_Impl.getFolderContent method:

Compiler allocated 4MB to compile java.util.List com.nextcloud.client.database.dao.FileDao_Impl.getFolderContent(long)

This influences the results significantly. Previously I had not paid attention to this message, so I got skewed results depending on the moment the method got optimized. 😞

[codecov]

Codecov Report

Merging #11227 (ec2759e) into master (6cb4a65) will increase coverage by 27.70%.
The diff coverage is 100.00%.

❗ Current head ec2759e differs from pull request most recent head d3841cd. Consider uploading reports for the commit d3841cd to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #11227       +/-   ##
=============================================
+ Coverage      3.24%   30.95%   +27.70%     
- Complexity      437     3317     +2880     
=============================================
  Files           566      565        -1     
  Lines         41614    41592       -22     
  Branches       5630     5637        +7     
=============================================
+ Hits           1349    12873    +11524     
+ Misses        40179    26786    -13393     
- Partials         86     1933     +1847     

Impacted Files	Coverage Δ
...loud/android/datamodel/FileDataStorageManager.java	61.38% <100.00%> (+61.29%)	⬆️
.../java/com/owncloud/android/datamodel/GalleryRow.kt	50.00% <0.00%> (ø)
...a/com/owncloud/android/media/MediaControlView.java	0.00% <0.00%> (ø)
...om/owncloud/android/ui/activity/EditorWebView.java	0.00% <0.00%> (ø)
...loud/android/utils/svg/SvgSoftwareLayerSetter.java	0.00% <0.00%> (ø)
.../android/ui/dialog/ChooseTemplateDialogFragment.kt	0.00% <0.00%> (ø)
...ndroid/ui/activity/RichDocumentsEditorWebView.java	0.00% <0.00%> (ø)
...g/java/com/nextcloud/test/InjectionTestActivity.kt
.../src/debug/java/com/nextcloud/test/TestActivity.kt
...rc/debug/java/com/nextcloud/client/TestActivity.kt	64.00% <0.00%> (ø)
... and 289 more

[AlvaroBrey]

At some point after starting the app, the JIT compiler decides to optimize the FileDao_Impl.getFolderContent method

If this kind of thing interests you, you may want to look into Baseline profiles so that critical methods like this one get compiled at install time.

[starypatyk]

If this kind of thing interests you, you may want to look into Baseline profiles so that critical methods like this one get compiled at install time.

Interesting indeed. 😉 For now, there are still a few potential "regular" improvements in the pipeline. Would be good to learn this at some point.

I had to partially revert the previous commit and leave the check for directories. I hope this will also fix the failing testRenameFolder test.

If I see correctly the testRenameFolder test is passing now. But I am struggling to find the results...

[starypatyk]

I have repeated the baseline tests and tests with the changes from this PR - this time making sure that in all cases the tests are executed after JIT decides to optimize the FileDao_Impl.getFolderContent method.

Method	Baseline - avg ms	PR - avg ms
OCFileListFragment::listDirectory	291	234
FileDataStorageManager::getFolderContent	168	112

We are gaining ~55 ms when skipping ~330 file.exists calls.

[AlvaroBrey]

When a folder is synced locally storagePath is set for files, but not for directories

I wonder why it's computed but not saved. @tobiasKaminsky any insights here?

Other than that, this is 👍 for me

[AlvaroBrey]

OK to merge this for now, thanks @starypatyk . I still don't know why the path is not stored for folders, but we'll go ahead with the current check.

[AlvaroBrey]

/rebase

[github-actions]

Codacy

Lint

Type	master	PR
Warnings	76	76
Errors	0	0

SpotBugs

Category	Base	New
Bad practice	27	27
Correctness	44	44
Dodgy code	335	335
Internationalization	9	9
Multithreaded correctness	9	9
Performance	58	58
Security	18	18
Total	500	500

[github-actions]

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11227.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]

master-IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/7495-IT-master-19-41

[AlvaroBrey]

Thanks @starypatyk !