Skip to content

perf: improve release validation performance#279

Open
letFunny wants to merge 7 commits into
canonical:mainfrom
letFunny:performance-improvements
Open

perf: improve release validation performance#279
letFunny wants to merge 7 commits into
canonical:mainfrom
letFunny:performance-improvements

Conversation

@letFunny
Copy link
Copy Markdown
Collaborator

@letFunny letFunny commented Apr 1, 2026

Avoid expensive calls to look for conflicts when the prefixes do not
match.

  • Have you signed the CLA?

I tried many performance improvements and rewrites of the logic below. This are the only ones that move the needle significantly. With big rewrites of the algorithm I am able to get a ~6% more, so in my opinion it is not worth going that route of extra complexity and longer reviews; we should look for performance improvements elsewhere. I also tried many other tricks like finding the end index by using binary search as well, being more clever about the prefix, etc. and none of them increased performance more than 2/3% compared to this PR, so again they were not included as they made the code more complex.

Avoid expensive calls to look for conflicts when the prefixes do not
match.
@letFunny
Copy link
Copy Markdown
Collaborator Author

letFunny commented Apr 1, 2026

The comment is not posted, I have downloaded the numbers from the CI artifacts:

| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `BASE` | 13.901 ± 0.136 | 13.709 | 14.233 | 3.11 ± 0.07 |
| `HEAD` | 4.477 ± 0.087 | 4.391 | 4.619 | 1.00 |

So a 3x improvement.

@letFunny letFunny marked this pull request as draft April 1, 2026 11:08
@letFunny letFunny marked this pull request as ready for review April 1, 2026 14:08
@letFunny letFunny requested a review from upils April 21, 2026 07:38
Copy link
Copy Markdown
Collaborator

@upils upils left a comment

Choose a reason for hiding this comment

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

Thanks @letFunny, this is really great to see such big performance gain!
It looks great overall but I am a bit concerned about the growing complexity of this whole operation.

Comment thread internal/setup/setup.go Outdated
Comment thread internal/setup/setup.go Outdated
Comment thread internal/setup/setup.go Outdated
Comment thread internal/setup/setup.go Outdated
Comment thread internal/setup/setup.go
Comment on lines +336 to +337
} else {
break
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I cannot confidently tell if this is changing anything. Is that to speed things up by skipping remaining slices in newSlices?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, once GlobDistance fails we can skip the rest of the loop. This obviously does not have a very big impact, compared to the other change, but it was a low hanging fruit for a 1 line change so I decided to add it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks! Maybe the comment could mention why this is fine to stop the rest of the loop?

Copy link
Copy Markdown
Collaborator Author

@letFunny letFunny Apr 23, 2026

Choose a reason for hiding this comment

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

I did and I added a TODO comment saying that the loops can be reordered. I didn't do it in these patches because:

  1. It did not have a measurable impact.
  2. It made the diff much bigger and harder to review.

Let me know what you think.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is much clearer, thanks!

@upils upils added the Polish Refactorings, etc label Apr 21, 2026
letFunny and others added 3 commits April 22, 2026 09:28
Co-authored-by: Upils <5464641+upils@users.noreply.github.com>
Co-authored-by: Upils <5464641+upils@users.noreply.github.com>
Copy link
Copy Markdown
Collaborator

@upils upils left a comment

Choose a reason for hiding this comment

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

Thanks. There is only a minor typo in a comment, but nothing blocking.

Comment thread internal/setup/setup.go Outdated
Co-authored-by: Upils <5464641+upils@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Please save this PR for later. This is not the time to fiddle with this complex and non-trivial algorithm, even more if the gains are marginal. From the last conversation on this, we want to improve the performance of the application as a whole by orders of magnitude, so if that's doing it, please let me know, otherwise let's keep talking.

@niemeyer
Copy link
Copy Markdown
Contributor

Sorry, I spoke too quickly.. I now see your follow up report above that this does improve by 3x, which is significant. Let's please look at this during or after the sprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Polish Refactorings, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants