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

Multiplatform benchmarks baseline #4350

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

qwwdfsad
Copy link
Collaborator

@qwwdfsad qwwdfsad commented Feb 12, 2025

I needed this for Kotlin/kotlinx-atomicfu#499 and decided to tidy it up a bit and make it usable.

Adding @fzhinkin to verify the sourcesets setup

@qwwdfsad qwwdfsad changed the title Fix basic compilation of benchmarks Multiplatform benchmark baseline Feb 12, 2025
@qwwdfsad qwwdfsad changed the title Multiplatform benchmark baseline Multiplatform benchmarks baseline Feb 12, 2025
@qwwdfsad qwwdfsad marked this pull request as ready for review February 12, 2025 13:05
@dkhalanskyjb
Copy link
Collaborator

I see that the jvmBenchmark/README.md was renamed, but no new README.md was introduced to replace it. How does one run SharedFlowBaseline?

"--add-exports", "java.base/sun.security.action=ALL-UNNAMED",
"--add-exports", "java.base/jdk.internal.util=ALL-UNNAMED") // in the model checking mode
"--add-exports", "java.base/jdk.internal.util=ALL-UNNAMED"
) // in the model checking mode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pro tip: use Difftastic when reviewing such changes! In a structural diff, the trivial formatting edits don't show up at all.

@dkhalanskyjb dkhalanskyjb self-requested a review February 12, 2025 13:37
dependencies {
implementation("org.jetbrains.kotlinx:kotlinx-benchmark-runtime:${version("benchmarks")}")
}
dependsOn(bmm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is bmm needed? Is using benchmarkMain (defined above) worse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

kotlin.srcDir("benchmarks/main/kotlin")
}

ss.create("jvmBenchmark") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the inconsistency with val benchmarkMain come from? Why not val jvmBenchmark by creating {?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -58,6 +60,7 @@ kotlin {
implementation("org.openjdk.jol:jol-core:0.16")
}
}
setupBenchmarkSourceSets(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a separate function? Simply for the sake of readability, or are there other motives?

Also, I'd prefer setupBenchmarkSourceSets(sourceSets) outside the sourceSets block, so that it's clearer what gets passed as the parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the sake of readability, to confine benchmark configuration which is far from trivial.
If it were benchmarks { jvm {} ...}, I would have left it here

@dkhalanskyjb dkhalanskyjb self-requested a review February 12, 2025 15:55
./gradlew :kotlinx-coroutines-core:macosArm64BenchmarkBenchmark
// Figure out what to use
./gradlew :kotlinx-coroutines-core:tasks | grep -i bench
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
./gradlew :kotlinx-coroutines-core:tasks | grep -i bench
./gradlew :kotlinx-coroutines-core:tasks | grep -i "BenchmarkBenchmark "

Seems much more reliable.

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