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

Chat application benchmark using Kotlin with Ktor framework (Closes #428) #442

Open
wants to merge 10 commits into
base: bench-devel/kotlin-ktor
Choose a base branch
from

Conversation

zavidnyi
Copy link

No description provided.

Copy link
Collaborator

@farquet farquet left a comment

Choose a reason for hiding this comment

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

Thanks a lot @zavidnyi for the contribution!

The benchmark looks very much in line with the spirit of the suite and the implementation matches well the rest of the suite.

README.md Outdated Show resolved Hide resolved
benchmarks/kotlin-ktor/README.md Outdated Show resolved Hide resolved
benchmarks/kotlin-ktor/README.md Outdated Show resolved Hide resolved
benchmarks/kotlin-ktor/README.md Outdated Show resolved Hide resolved
benchmarks/kotlin-ktor/README.md Outdated Show resolved Hide resolved
benchmarks/kotlin-ktor/README.md Outdated Show resolved Hide resolved
@zavidnyi
Copy link
Author

Hey @farquet,
I tried to clarify the benchmark readme.
Let me, know if it's more clear now or if there any other issues

@lbulej
Copy link
Member

lbulej commented May 14, 2024

Ilia (@zavidnyi), could you look into why the JMH wrappers fail to build in the CI? The JMH wrapper around the Kotlin benchmark should not really need to run kotlinCompile task, so maybe just disabling the plugin for the wrappers target could fix that?

@zavidnyi
Copy link
Author

@lbulej I've fixed an issue by disabling the Koltin plugin for both renaissanceJmhWrappers and dependant renaissanceJmh.
However, this seems like more of a workaround rather than a fix.
I'm unfamiliar with sbt, so it's hard for me to say why koltinCompile even triggers there.

@lbulej
Copy link
Member

lbulej commented May 22, 2024

@lbulej I've fixed an issue by disabling the Koltin plugin for both renaissanceJmhWrappers and dependant renaissanceJmh.

Thanks!

However, this seems like more of a workaround rather than a fix. I'm unfamiliar with sbt, so it's hard for me to say why koltinCompile even triggers there.

No problem, that's the usual case with SBT :-) I'll check later if there is a better way to do this, but I want to merge the benchmark in a state that passes the CI.

One (I believe) last thing: please add test and jmh configurations to the benchmark — see for example RxScrabble.scala. The test configuration is supposed to trigger a short workload meant to be run during CI. The jmh configuration is used when run via JMH and is usually empty (meaning it uses the defaults) — unless there is a need for an override under JMH.

@zavidnyi
Copy link
Author

@lbulej I've added the test and jmh configurations for the kotlin-ktor

One question:
Where is exactly jmh configuration is used?
I've not found any place in the repository which calls it and according to the README I'd assume that I can pass arguments to the JMH package as usual, without extra empty configuration

@lbulej
Copy link
Member

lbulej commented May 23, 2024

@lbulej I've added the test and jmh configurations for the kotlin-ktor

Thanks!

One question: Where is exactly jmh configuration is used? I've not found any place in the repository which calls it and according to the README I'd assume that I can pass arguments to the JMH package as usual, without extra empty configuration

It is used (by default) by the JmhRenaissanceBenchmark class, which is the base class for JMH wrapper classes that are generated for individual benchmarks.

@zavidnyi
Copy link
Author

Hi @lbulej, I've just noticed that PR still not merged.
I can see that some checks failed, but for some reason I can't see the details.
Could you please rerun them, so I could check what's wrong?

@lbulej
Copy link
Member

lbulej commented Jun 14, 2024

Hi @lbulej, I've just noticed that PR still not merged. I can see that some checks failed, but for some reason I can't see the details. Could you please rerun them, so I could check what's wrong?

The JVM has trouble finding some Kotlin classes when run under JMH. I have re-run the checks so that you could see for yourself (you should be able to run the CI checks in your repository too).

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