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

[WIP] Improve benchmark pipeline #184

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gavlyukovskiy
Copy link
Collaborator

@gavlyukovskiy gavlyukovskiy commented Sep 25, 2024

Adding benchmarks pipeline based on this action: https://github.com/marketplace/actions/continuous-benchmark

The current pipeline is to backfill all historical benchmark data, which prompted me to do some interesting stuff with the code, so ignore removing nullaway and adding old jsr305 dependencies, this all is needed to be able to compile latest benchmarks together with the historical code 🙃
I also tried to unify all benchmarks to use the same parameters, so we have something that's consistent.

@Breus @donavdey please review the changes to benchmarks (parameters and json path changes), ignoring all the others for now. If we need any changes to the benchmarks and backfill the historical data, then we will need all these hacks to rerun them.

The benchmarks are pushed into https://gavlyukovskiy.github.io/json-masker-benchmarks/, but I plan to change that to json-masker repository, for that we need to enable github-pages for the repository and push into gh-pages branch used to render it (and also remove my clear-text PAT 😅).

The pipeline will need to be configured to only push results from master branch and comment on PRs, it has a couple of options like commenting on specific threshold or always, but it requires merging the pipeline into master to test it out. Or we can play with it in a separate repository.

@Breus
Copy link
Owner

Breus commented Sep 26, 2024

@gavlyukovskiy would it be nice if I prepare json-masker.blaauwendraad.dev to publish benchmark results and maybe some kind of public documentation page? We can also host our own JavaDoc there :-)

@gavlyukovskiy
Copy link
Collaborator Author

gavlyukovskiy commented Sep 26, 2024

@Breus yes, please enable gh pages (some repo setting, I think), you will need to commit empty .nojekyll into the root of gh-pages branch and then I can transfer everything from my repository (https://github.com/gavlyukovskiy/json-masker-benchmarks) into that one. We don't have to use custom domain as after enabling Github Pages we will have https://breus.github.io/json-masker/ for free, but we can do custom domain as well :)

We could also reuse the same PAT to publish this as we do for gradle wrapper updates (needs additional "pages" permission).

I also had the same thoughts about javadoc 😁 I think we could do that, but also not much is wrong with our current service. In any case, we can use /benchmarks for the benchmarks and leave a room for something else later.

@@ -15,6 +22,25 @@

public class BenchmarkUtils {

public static final long STATIC_RANDOM_SEED = 1285756302517652226L;
public static final int TARGET_KEYS_SIZE = 2385;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the number of keys we have in target_keys.json, I just needed the precise number somewhere

@donavdey
Copy link
Collaborator

donavdey commented Oct 3, 2024

It also probably makes sense to remove src/jmh/benchmark-history folder as the history will be stored somewhere else.

@gavlyukovskiy
Copy link
Collaborator Author

@donavdey yea, it's a long time irrelevant at this point

Copy link

sonarqubecloud bot commented Oct 3, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants