Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve runtime of delta computation #553

Merged
merged 4 commits into from
Jan 11, 2021
Merged

Conversation

abramobagnara
Copy link

@abramobagnara abramobagnara commented Jan 7, 2021

Currently the difference of reports use a quadratic algorithm take takes hours when issues are several thousands.
The changes done copes with that.

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #553 (90ea0d7) into master (0544a8d) will increase coverage by 0.01%.
The diff coverage is 96.29%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #553      +/-   ##
============================================
+ Coverage     90.46%   90.48%   +0.01%     
- Complexity     1586     1590       +4     
============================================
  Files           177      177              
  Lines          4899     4919      +20     
  Branches        542      546       +4     
============================================
+ Hits           4432     4451      +19     
  Misses          293      293              
- Partials        174      175       +1     
Impacted Files Coverage Δ Complexity Δ
...n/java/edu/hm/hafner/analysis/IssueDifference.java 98.46% <96.29%> (-1.54%) 24.00 <9.00> (+4.00) ⬇️
src/main/java/edu/hm/hafner/analysis/Issue.java 77.40% <0.00%> (ø) 76.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0544a8d...be1cc69. Read the comment docs.

src/main/java/edu/hm/hafner/analysis/IssueDifference.java Outdated Show resolved Hide resolved
src/main/java/edu/hm/hafner/analysis/IssueDifference.java Outdated Show resolved Hide resolved
referencesByHash = new HashMap();
referencesByFingerprint = new HashMap();

for (Issue issue : fixedIssues) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More readable:

Suggested change
for (Issue issue : fixedIssues) {
for (Issue issue : referenceIssues) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same and would case regression test failures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they are copied above

@@ -57,12 +70,31 @@ private void matchIssuesByFingerprint(final Report currentIssues) {
}
}

private <K> void addIssueToMap(final Map<K, List<Issue>> map, final K key, final Issue issue) {
List<Issue> issues = map.get(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use putIfAbsent

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would put more pressure on GC creating possibly unneeded object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #555

}
return Optional.empty();
}

private List<Issue> findReferenceByEquals(final Issue current) {
List<Issue> references = referencesByHash.get(current.hashCode());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
List<Issue> references = referencesByHash.get(current.hashCode());
List<Issue> references = referencesByHash.getOrDefault(current.hashCode(), new ArrayList<>());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would put more pressure on GC creating possibly unneeded object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #555

for (Issue reference : fixedIssues) {
if (current.equals(reference)) {
equalIssues.add(reference);
if (references != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed if default is returned

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #555

@uhafner uhafner changed the title Avoid quadratic times. Improve runtime of delta computation Jan 8, 2021
@uhafner uhafner added the bug Bugs or performance problems label Jan 8, 2021
@uhafner uhafner merged commit be1cc69 into jenkinsci:master Jan 11, 2021
@uhafner uhafner added enhancement Enhancement of existing functionality and removed bug Bugs or performance problems labels Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants