Defer expensive operations until results are read from OCFile

[starypatyk]

As discussed in #10783, this PR is an attempt to defer relatively expensive operations performed during creation of the OCFile objects - esp. affecting the FileDataStorageManager::getFolderContent method.

The first part is deferring JSON deserialization of sharees and imageDimension members of OCFile.

This change gives a modest speed improvement. In my "standard" benchmark (~330 files in a directory on a low-powered device) the average execution time of the OCFileListFragment::listDirectory call is reduced from 340 ms to 300 ms.

@AlvaroBrey @tobiasKaminsky - Some questions:

• Are the OCFile objects shared between threads? If yes then I should probably add synchronization to the new get/set methods.
• The second part would be removing the File.exists check. Do you think this should be included in this PR or should I submit a separate one?

[github-actions]

Codacy

Lint

Type	master	PR
Warnings	82	82
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/11181.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/7161-IT-master-20-59

[github-actions]

blue-Light-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11181-Screenshot-blue-Light-21-05

[tobiasKaminsky]

The first part is deferring JSON deserialization of sharees and imageDimension members of OCFile.

I wrote it also in the other issue: would it also working to use a real database for this?
Then we would not have to think if objects are used in multiple threads, etc.

[AlvaroBrey]

Hey @starypatyk , we've discussed this and, as mentioned by Tobi, we'd rather do the parsing when saving the object to the database (well, actually when getting it from the server would be more correct). This avoids inconsistencies altogether, and it's how it should've been modeled in the first place.

To do this we'll need to store the sharees in a separate table with a foreign key to the filelist table. Not entirely sure how that's done in Room, but I think it is possible.

imageDimension, unless I'm missing something, could just be stored as two separate floating point columns, to avoid serialization and deserialization. We can even keep the ImageDimension type in OCFile and just have it as separate values in the entity.

• The second part would be removing the File.exists check. Do you think this should be included in this PR or should I submit a separate one?

Separate PR would be better. Thanks!

[starypatyk]

@AlvaroBrey

Starting from the simple one...
At least simple to implement, I am not sure about the testing. 😉

• The second part would be removing the File.exists check. Do you think this should be included in this PR or should I submit a separate one?

Separate PR would be better. Thanks!

I have submitted PR #11227.

[starypatyk]

@AlvaroBrey @tobiasKaminsky

Hey @starypatyk , we've discussed this and, as mentioned by Tobi, we'd rather do the parsing when saving the object to the database (well, actually when getting it from the server would be more correct). This avoids inconsistencies altogether, and it's how it should've been modeled in the first place.

To do this we'll need to store the sharees in a separate table with a foreign key to the filelist table. Not entirely sure how that's done in Room, but I think it is possible.

imageDimension, unless I'm missing something, could just be stored as two separate floating point columns, to avoid serialization and deserialization. We can even keep the ImageDimension type in OCFile and just have it as separate values in the entity.

As a short-term solution, I proposed a separate PR #11251.

Thinking long-term: fully agree with the idea of having two numeric fields for the image dimensions.

Regarding sharees - separating this data into another table seems indeed to be the most elegant solution. I am not sure however, that such approach is going to improve performance. I am quite surprised that JSON decoding takes that much time. As this is purely in-memory operation, I would suspect JSON decoding to be much faster than database retrieval - which involves I/O operations.

Making such change would require some careful performance verification, to make sure that it indeed improves performance.

BTW - I see some data redundancy between the sharees field in the filelist table and the ocshares table. I plan another PR in this area - related to the FileDataStorageManager::saveSharesInFolder. This will probably be a much better place to continue this discussion.

Closing this PR as no longer relevant.