Merging Vighnesh's FYP work

[damithc]

@vig42 @fzdy1914 It's best to speed up merging of Vighnesh's FYP work as we need time to field-test the stability of the work before the semester is over. We don't want to merge all his work at once either; it's better to merge one, wait for it to stabilize, and then merge the next.

[fzdy1914]

OK, I will try to merge #1449 first and try to have a field test for that.

[fzdy1914]

@Tejas2805 Your help is also needed here.

[Tejas2805]

Yup, just reviewed. Sorry for the delay. Been busy with my own FYP :( .

[damithc]

These are major changes and the dev is graduating. So, need to be very thorough/deep in the review (i.e., rather than cosmetic things, focus more on long term implications and underlying design decisions). No need to merge if we are not 100% sure if this is something we want in RepoSense. FYP projects are assumed 'experimental' and merging is not a must.

[vig42]

If the benefit is worth the complexity, I don't mind having it. Getting exposed to a bit of multi-threading code is not a bad thing for our students. @vig42

Right now, the bulk of the changes introduced in the multi-threading PR are in the cloneAndAnalyzeRepos method in ReportGenerator.java.

The multi-threading code is a little confusing, but it really just boils down to these 4 steps:

1. We first create ExecutorService objects to use a fixed thread pool to cap the number of threads used for cloning and analysis

        ExecutorService cloneExecutor = Executors.newFixedThreadPool(numCloningThreads);
        ExecutorService analyzeExecutor = Executors.newFixedThreadPool(numAnalysisThreads);

2. We create a CompletableFuture for each repo location. For each of the locations, we use supplyAsync to run the cloneRepo function in parallel for each repo. The cloneExecutor input ensures that this job is subject to the thread cap for cloning.

        List<CompletableFuture<CloneJobOutput>> cloneJobFutures = repoLocationList.stream()
                .map(location -> CompletableFuture.supplyAsync(() ->
                        cloneRepo(repoLocationMap, location), cloneExecutor))
                .collect(Collectors.toList());

3. Then we use the thenApplyAsync method of the CompletableFutures to run the analyseRepos function for each repo. This ensures that the analysis step for a specific repo is only done after it has been cloned first. Note that we have to collect after the last step and use another stream for this.

        List<CompletableFuture<AnalyzeJobOutput>> analyzeJobFutures = cloneJobFutures.stream()
                .map(cloneFuture -> cloneFuture.thenApplyAsync(cloneJobOutput ->
                        analyzeRepos(outputPath, repoLocationMap, cloneJobOutput), analyzeExecutor))
                .collect(Collectors.toList());

4. Finally we collect the outputs from all the jobs.

        List<AnalyzeJobOutput> jobOutputs = analyzeJobFutures.stream()
                .map(CompletableFuture::join)
                .collect(Collectors.toList());
        cloneExecutor.shutdown();
        analyzeExecutor.shutdown();

        List<Path> generatedFiles = jobOutputs.stream()
                .flatMap(jobOutput -> jobOutput.getFiles().stream())
                .collect(Collectors.toList());

Besides that, there is 1 other small change relative to the master branch. Basically, when there is a cloning or analysis failure for a particular repo, RepoSense will remove the respective RepoConfiguration from the configs list. Currently, this is done immediately after the respective cloning or analysis step fails. However, with multi-threading enabled, since the configs list is shared across all the threads, we could face race conditions if different threads experience failures and try to modify the list at the same time.

To overcome this, I have changed it so that RepoSense does the error handling at the end, after all the jobs are completed. To aid with this I have created a new data structure class AnalysisErrorInfo to store the error messages (if any). So once all the multi-threaded cloning and analysis jobs are completed, RepoSense will go through the outputs, and if there were any errors, it will call the respective functions handleCloningFailed and handleAnalysisFailed (these functions were not modified).

• See if the complexity is (or can be) localized into some areas and new developers can ignore those parts at first

As it stands, I think that developers can mostly ignore the added complexity here. If they are working on anything related to cloning or analysis, the only thing they would need to take note of is that the files for each repo (this includes the cloned repo folder, the JSON outputs from analysis and anything new) should be in distinct folders. The cloning & analysis functions for each repo should not touch files related to other repos.

• Is there any way to reduce or isolate the additional complexity?

I feel that using the inbuilt ExecutorService and CompletableFuture classes has already reduced the complexity quite a lot. I am not sure if it is possible to make the multi-threading part even simpler.

If you feel that it would be beneficial, I can add some comments describing each of the steps that I listed above.

[damithc]

If you feel that it would be beneficial, I can add some comments describing each of the steps that I listed above.

@vig42 Yes, I think that would be good. Add all these additional information into the case as comments where appropriate. I think it is OK to over-comment this part of the code. Note that the comments should explain the current state of the code, not how it was changed from the previous state. The latter explanation should be in the commit message, not the code comments.

[Tejas2805]

@damithc The explanation given by @vig42 seems to be quite clean and clear. It might be good to have this in our DG under Design and implementation for new developers to go through or we can leave it here for them to go through it. Your thoughts?

[damithc]

If the explanation can be in the form of an elaborate code comment, it is easier to maintain and only those who come into contact with that part of the code needs to read it. However, if the relevant code is in many places, we may have to document the whole thing in the DG so that the explanation is in one place rather than scattered as code comments in multiple files.

[vig42]

If the explanation can be in the form of an elaborate code comment, it is easier to maintain and only those who come into contact with that part of the code needs to read it. However, if the relevant code is in many places, we may have to document the whole thing in the DG so that the explanation is in one place rather than scattered as code comments in multiple files.

Since the relevant code is all in 1 function, I have provided the explanation in code comments in ReportGenerator.java.

I have also updated the DG under Architecture. I have mentioned that multi-threading is used for cloning and analysis of repos, and mentioned the 2 hidden CLI flags --cloning-threads and --analysis-threads which can be used to control the number of threads.