tsantalis / RefactoringMiner

MIT License
372 stars 138 forks source link

More progress output #765

Closed koppor closed 2 months ago

koppor commented 2 months ago

When starting RFM, I first get one line that it is connected. And one minute alter some progress output. (URL: https://github.com/JabRef/jabref/pull/11542)

[main] INFO org.refactoringminer.rm1.GitHistoryRefactoringMinerImpl - Connected to GitHub with OAuth token
[pool-2-thread-1] INFO org.refactoringminer.astDiff.matchers.ProjectASTDiffer - ModelDiff.getRefactorings() execution time: 71 seconds

Last time I tried, it was only a few seconds...

Maybe, some progressbar could be used while "ModelDiff.getRefactorings()" is running?

tsantalis commented 2 months ago

@koppor

The processing time will obviously change as more files are added in the PR. From what I see the PR has quite many unmatched files (18 deleted and 18 added java files).

This introduces more processing for the tool, as it tries to find potentially moved code between the deleted and added files.

I found an interesting refactoring in FulltextSearchResultsTab.java https://github.com/JabRef/jabref/pull/11542/files#diff-458487147da084adaa1b70eb4bb86fb2ce15e4c0d16eb19465eb97bf8aec8905R97-R122

which our tool does not currently support. There is some re-structuring of the lambda expressions.

tsantalis commented 2 months ago

The matching of files src/main/java/org/jabref/logic/pdf/search/IndexingTaskManager.java src/main/java/org/jabref/logic/search/LuceneManager.java seems wrong

koppor commented 2 months ago

The matching of files src/main/java/org/jabref/logic/pdf/search/IndexingTaskManager.java src/main/java/org/jabref/logic/search/LuceneManager.java seems wrong

That matching is right 😅

-import org.jabref.logic.pdf.search.IndexingTaskManager;
+import org.jabref.logic.search.LuceneManager;
...
-        super(false, databaseContext, suggestionProviders, undoManager, undoAction, redoAction, dialogService, preferences, themeManager, taskExecutor, journalAbbreviationRepository, indexingTaskManager, searchQueryProperty);
+       super(false, databaseContext, suggestionProviders, undoManager, undoAction, redoAction, dialogService, preferences, themeManager, taskExecutor, journalAbbreviationRepository, luceneManager, searchQueryProperty);

Source: https://github.com/JabRef/jabref/pull/11542/files#diff-e332bc02dafca884fa0cb9109501babc5c9b8c17dc2ed9fca8d426ef6e916323

tsantalis commented 2 months ago

Mapping improvement in FulltextSearchResultsTab.java Fix for #766

Screenshot from 2024-08-28 15-48-35

tsantalis commented 2 months ago

Another possible improvement can be done in src/main/java/org/jabref/gui/frame/JabRefFrameViewModel.java by matching

parserResults.forEach(pr -> and .ifPresent(

A lambda with body can be matched with a lambda with expression

image

AFTER THE FIX

image

tsantalis commented 2 months ago

Improve detection of Extract to Inner class in file src/main/java/org/jabref/gui/maintable/MainTableDataModel.java

image

AFTER THE FIX

image

tsantalis commented 2 months ago

Problem in attribute matching src/test/java/org/jabref/logic/search/DatabaseSearcherWithBibFilesTest.java

image

AFTER THE FIX

image

tsantalis commented 2 months ago

@koppor

The problem with progressbar is that we don't beforehand what is the size of the task. We know how many files we need to diff, but after diffing the files with the same file path (1st step), we have to find moved/renamed files (2nd step), and then as a final step find moved code between files (3rd step).

The size of the 2nd and 3rd steps, depends on the output of the 1st step. We cannot use a progressbar with a fixed size, unless we can dynamically update the size of the task.

koppor commented 2 months ago

@koppor

The problem with progressbar is that we don't beforehand what is the size of the task.

Allright.

The size of the 2nd and 3rd steps, depends on the output of We cannot use a progressbar with a fixed size, unless we can dynamically update the size of the task.

Therefore I proposed the linked library above.

From their README.md:

pb.maxHint(n);
    // reset the max of this progress bar as n. This may be useful when the program
    // gets new information about the current progress.
    // Can set n to be less than zero: this means that this progress bar would become
    // indefinite: the max would be unknown.
tsantalis commented 2 months ago

OK, I see. It is possible to dynamically adjust the size.

The implementation is quite challenging, because there are many files involved, and it is hard to coordinate the progression. I will have to think about it..

koppor commented 2 months ago

I would really not care if the progess "jumps". Some progress indication would be better then none ^^.

Reason: I was used that RFM is fast as hell - and now it was slow as never seen before ^^

tsantalis commented 2 months ago

@koppor We don't see any performance slowdown in our RefactoringMiner and code-tracker tests. They take the same time to finish as before.

Probably you are reviewing larger PRs lately with moves between files. This is something that slows down the tool. If there are moves between files, there might be some combinatorial explosion, as all possible combinations of deleted/added methods are attempted to be matched.

koppor commented 2 months ago

Probably you are reviewing larger PRs lately with moves between files.

Yes. I remember you explaining back then... It was the first time I saw RFM being slow 😅