Skip to content

GH-273 add benchmarks #274

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

Closed
wants to merge 1 commit into from

Conversation

hmottestad
Copy link

Add benchmarks.

@hmottestad hmottestad force-pushed the GH-273-add-benchmark branch from cf0eacb to 5c53ad2 Compare August 15, 2023 10:01
@hmottestad
Copy link
Author

@filip26 I assume you're on holiday :) Norway has holidays in July, but the rest of Europe typically has in August. Just wanted to let you know that this PR is the first step in my plan for merging in the performance optimizations. Each PR will build on the previous, so I won't start preparing the next one until this one is merged. That way we can measure the impact of each PR.

@hmottestad
Copy link
Author

Any chance you've had a chance to look at this PR @filip26 ?

@filip26
Copy link
Owner

filip26 commented Sep 25, 2023

Hi @hmottestad ,
I'm sorry for the long wait, I will try to find time this weekend.

@hmottestad
Copy link
Author

@filip26 have you had a chance to look this over yet?

@hmottestad hmottestad force-pushed the GH-273-add-benchmark branch from 5c53ad2 to 887ed47 Compare October 24, 2023 11:59
@filip26
Copy link
Owner

filip26 commented Jan 22, 2024

@hmottestad for some reason I wasn't able commit to the PR directly, here is a branch resolving all the conflicts, please check it and update the PR. Thank you.

I'm thinking about using Github actions and virtual machines to run the benchmarks. What do you think? Is there an action we can use to run it? Or would you be willing to write it? or do you have other thoughts?

https://github.com/filip26/titanium-json-ld/tree/GH-273-add-benchmark

@hmottestad
Copy link
Author

I usually prefer comments on the PR that I can resolve. I can't really compare your branch with mine, could you explain what you've changed?

@filip26
Copy link
Owner

filip26 commented Jan 22, 2024

@hmottestad sorry, as you can see the PR has a conflict with main branch, pom_parent.xml has changed. I've only resolved the conflict, that's all.

@hmottestad
Copy link
Author

Ahh, ok, I can fix that by rebasing onto main.

My experience with running the benchmarks is simply to run them locally on my laptop on the main branch and then on the branch that I'm working on to compare the performance. I've not heard of anyone running JMH benchmarks with Github Actions. I know the JMH can output the benchmark results as JSON, so you could try to write a custom Action to run it before and after and then parse the json and create some kind of comparison.

One general issue with using Github Actions is that there is no way to control what hardware you run on or who else is running at the same time.

@filip26
Copy link
Owner

filip26 commented Jan 22, 2024

The issue with running benchmarks locally is that the results cannot be compared, as there is no reference machine configuration. Also, it complicates an approval, as a maintainer has to run it locally too, possibly getting different results.

I have no deep knowledge of GitHub infrastructure, even I would assume they use the same configuration across (public) repositories. Which is not good assertion, but better than using local machines where configuration will be totally different (not counting OS, other applications running in the background, etc.)

Well, if nobody uses it, perhaps, there is an opportunity for some to get a credit, and maybe $, for leading ;)

@hmottestad
Copy link
Author

hmottestad commented Jan 22, 2024

I've usually trusted the contributors when they show a performance gain for a code change.

I'll leave making a GitHub Action to you.

@filip26
Copy link
Owner

filip26 commented Jan 22, 2024

Thanks for the generosity, but benchmarks are not my concern and my plate is full already ;)

@hmottestad hmottestad closed this Jan 22, 2024
@hmottestad
Copy link
Author

My understanding is that you want Github Actions and that you don't want to merge this PR without it. So I've gone ahead and closed this PR since neither of us wants to implement that. Please let me know if I've misunderstood.

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.

2 participants