Implemented Parallel Download of Forms using Multi-Threading and Locks #60

prabs3257 · 2023-06-06T07:56:56Z

Closes #7

What has been done to verify that this works as intended?

Tested the code by artificially increasing the download time by adding delays, results show that the code works as intended. Verified that all the form files are being downloaded and saved properly.

Why is this the best possible solution? Were any other approaches considered?

Increases the performance of the app exponentially.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This change does not affect the user

Do we need any specific form for testing your changes? If so, please attach one.

No

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

…l and Added an Initial-Setup Guide.

chinmoy12c · 2023-06-07T13:37:58Z

...t/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java

 try {
 installEverything(tempMediaPath, fileResult, parsedFields, formsDirPath, newAttachmentsDetected);
+ lock.unlock();
 } catch (FormDownloadException.DiskError e) {
 cleanUp(fileResult, tempMediaPath);


You haven't released the lock in catch. This could lead to a deadlock if an exception occurs. This also needs to be handled in other catch blocks as well.

chinmoy12c · 2023-06-07T14:51:00Z

odk/collect/collect_app/src/main/java/org/odk/collect/android/tasks/DownloadFormsTask.java


 index++;
 }
+ executorService.shutdown();


executorService may shut down before all the threads have completed work, it is not a blocking method. In such a scenario results might not contain all the forms data.

chinmoy12c · 2023-06-07T14:53:47Z

odk/collect/collect_app/src/main/java/org/odk/collect/android/tasks/DownloadFormsTask.java

+ new Handler(Looper.getMainLooper()).post(new Runnable() {
+ @Override
+ public void run() {
+ results.put(serverFormDetails, null);


HashMaps are not thread safe.

chinmoy12c · 2023-06-07T14:55:35Z

odk/collect/collect_app/src/main/java/org/odk/collect/android/tasks/DownloadFormsTask.java

- }
+ String currentFormNumber = String.valueOf(index);
+ String totalForms = String.valueOf(values[0].size());
+ publishProgress(serverFormDetails.getFormName(), currentFormNumber, totalForms);


Since this process is not synchronous now, currentFormNumber will return wrong data when used in a concurrent threaded system.

chinmoy12c · 2023-06-07T15:00:54Z

...t/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java

 // download media files if there are any
+ lock.lock();


You've acquired the lock at the beginning of the process and released the lock at the end of the download process. What this essentially does is:

A new thread acquires the lock to download form media.

The thread downloads the form media by making a web request.

The thread releases the lock.

How is different than synchronous processing if the other threads are anyways waiting for the lock to be released while the download process is on?

chinmoy12c · 2023-06-07T15:02:59Z

...t/collect_app/src/main/java/org/odk/collect/android/formmanagement/ServerFormDownloader.java

@@ -94,8 +98,8 @@ private void processOneForm(ServerFormDetails fd, OngoingWorkListener stateListe
 // get the xml file
 // if we've downloaded a duplicate, this gives us the file
 fileResult = downloadXform(fd.getFormName(), fd.getDownloadUrl(), stateListener, tempDir, formsDirPath);


Are you sure this method is thread safe? If so, how?

prabs3257 · 2023-06-07T18:34:54Z

@chinmoy12c These changes should make the media downloads also parallel and should properly shut down the executorservice and I have also released the locks in the catch blocks also. Also used locks in the downloadXForm function so that its thread safe. Is this correct ?

chinmoy12c

We should also find a way to test these with a large number of forms. Could you set up such an environment and share the test results?

chinmoy12c · 2023-06-13T09:47:45Z

...lect/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt

-import org.odk.collect.forms.FormSourceException
-import org.odk.collect.forms.FormsRepository
-import org.odk.collect.forms.MediaFile
+import org.odk.collect.forms.*


Please remove * imports and use specific imports.

chinmoy12c · 2023-06-13T09:48:27Z

...lect/collect_app/src/main/java/org/odk/collect/android/formmanagement/FormMediaDownloader.kt


 class FormMediaDownloader(
 private val formsRepository: FormsRepository,
 private val formSource: FormSource
 ) {

+ private val lock: Lock = ReentrantLock()


nit, add a newline.

chinmoy12c · 2023-06-13T10:25:04Z

odk/collect/collect_app/src/main/java/org/odk/collect/android/tasks/DownloadFormsTask.java

+ executorService.shutdown();
+ // Wait for all tasks to complete
+ try {
+ executorService.awaitTermination(1, TimeUnit.MINUTES);


What was the basis for choosing 1 minute for timeout? If there are 500 forms, will those be completed in 500 forms?

chinmoy12c · 2023-06-13T10:30:22Z

odk/collect/collect_app/src/main/java/org/odk/collect/android/tasks/DownloadFormsTask.java

+ String totalForms = String.valueOf(values[0].size());
+ publishProgress(serverFormDetails.getFormName(), currentFormNumber, totalForms);
+
+ Thread thread = new Thread() {


How are we limiting resource consumption on this? If there are a large number of forms let's say 1000 forms, this would open 1000 threads which is not exactly resource friendly. We should provide a configurable way to limit the number of threads.

prabs3257 · 2023-06-15T23:01:39Z

@chinmoy12c @charanpreet-samagra I have set up my central project with 100 forms to simulate a demanding task. The proposed solution completes the download of all the forms in roughly 2 seconds (where download start time = 03:40:23.052 and end time = 03:40:25.000)

When the number of forms was increased in the project to 514. The proposed solution completes the download of all the forms in roughly 8 seconds (where download start time = 15:04:27.172 and end time = 15:04:35.285)

All these forms were simple forms with only one input field and no media attached. The bulk upload was done using ODK Central API and a self-written python script

This is the profiling when there are 514 forms to download

chinmoy12c · 2023-06-23T06:49:14Z

@chinmoy12c @charanpreet-samagra I have set up my central project with 100 forms to simulate a demanding task. The proposed solution completes the download of all the forms in roughly 2 seconds (where download start time = 03:40:23.052 and end time = 03:40:25.000)

When the number of forms was increased in the project to 514. The proposed solution completes the download of all the forms in roughly 8 seconds (where download start time = 15:04:27.172 and end time = 15:04:35.285)

All these forms were simple forms with only one input field and no media attached. The bulk upload was done using ODK Central API and a self-written python script

This is the profiling when there are 514 forms to download

This is great @prabs3257. Could you make the final changes that I requested?

Prabhav Chopra and others added 5 commits June 2, 2023 13:01

Added network_security_config.xml, Made Changes to AndroidManifest.xm…

f0f927a

…l and Added an Initial-Setup Guide.

Incorporated Intial-Setup.md into Readme.md

984ce3f

Made the review changes

4f865e6

Merge remote-tracking branch 'upstream/develop' into develop

ca951ff

implemented parallel download

9e20f12

chinmoy12c requested changes Jun 7, 2023

View reviewed changes

prabs3257 added 2 commits June 7, 2023 23:52

Fixed media download and shutdown problems

2e706dd

Merge branch 'Samagra-Development:develop' into parallel-test

0a4c00d

chinmoy12c requested changes Jun 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Parallel Download of Forms using Multi-Threading and Locks #60

Implemented Parallel Download of Forms using Multi-Threading and Locks #60

prabs3257 commented Jun 6, 2023

chinmoy12c Jun 7, 2023

chinmoy12c Jun 7, 2023

chinmoy12c Jun 7, 2023

chinmoy12c Jun 7, 2023

chinmoy12c Jun 7, 2023

chinmoy12c Jun 7, 2023

prabs3257 commented Jun 7, 2023 •

edited

Loading

chinmoy12c left a comment

chinmoy12c Jun 13, 2023

chinmoy12c Jun 13, 2023

chinmoy12c Jun 13, 2023 •

edited

Loading

chinmoy12c Jun 13, 2023

prabs3257 commented Jun 15, 2023 •

edited

Loading

chinmoy12c commented Jun 23, 2023

Implemented Parallel Download of Forms using Multi-Threading and Locks #60

Are you sure you want to change the base?

Implemented Parallel Download of Forms using Multi-Threading and Locks #60

Conversation

prabs3257 commented Jun 6, 2023

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

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

prabs3257 commented Jun 7, 2023 • edited Loading

chinmoy12c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmoy12c Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prabs3257 commented Jun 15, 2023 • edited Loading

chinmoy12c commented Jun 23, 2023

prabs3257 commented Jun 7, 2023 •

edited

Loading

chinmoy12c Jun 13, 2023 •

edited

Loading

prabs3257 commented Jun 15, 2023 •

edited

Loading