Kotlin/Native support #32

LepilkinaElena · 2021-02-15T14:03:47Z

No description provided.

build.gradle

plugin/build.gradle

runtime/build.gradle

ilya-g · 2021-02-17T16:24:39Z

README.md

@@ -166,6 +162,8 @@ Available configuration options:
 * `iterationTimeUnit` – time unit for `iterationTime` (default is seconds)
 * `outputTimeUnit` – time unit for results output
 * `mode` – "thrpt" for measuring operations per time, or "avgt" for measuring time per operation
+* `iterationMode` – "external" for iterating in gradle in order to get correct Kotlin/Native runtime input in measurement,
+ "internal" can be used if it's known that measured code have no calls in K/N runtime that can influence on measurement unrepeatedly.


From this description it's very hard to get when one should use this configuration option. Perhaps, if there's a sensible default, we could turn it into a boolean option that would allow to override this default and explain why users would want to do it.

"External" is already such default. It's similar to other settings that also have. default values, but about description I agree. If you have ideas what to write. in README, I'll be grateful

* iterationMode – way of iteration for K/N benchmarks. Default value - "external", each iteration is processed as separate run, allows to get more precise results in most cases. "internal" - all iterations of one benchmark run during one binary execution (pay attention, some code performance can't be measured properly using this mode, e.g. singleton initialization, because of implementation details of K/N runtime)

Is this better?

It feels conceptually like 'fork' option in JMH (see the example). Perhaps we could find a resembling name for this option.

singleton initialization

I always thought that microbenchmarks tried to measure the settled code execution performance, i.e. when all fixed initialization costs are done and no longer affect the performance of the code being measured. That's why they do warm-up iterations.
So for me it looks like that the mode of a fork-per-benchmark method should be the preferred default.

I thought during implementation about fork, but it's different in jmh. Firstly fork is number and user can set by user. Also as I undertood JMH created needed number of forks and warmups and iterations is made inside each fork, logic of working for "external" mode is another, forks are even more similar for "internal" mode where is one fork for each benchmark. I really think that using fork can confuse users, because they'll expect the same behaviour for Native.

when all fixed initialization costs are done and no longer affect the performance of the code being measured.

It isn't true for native runtime. Done initialization may influence on the next code. And we don't know what exactly user want to measure.

That's why they do warm-up iterations.

Warm-uo for native applications is needed to warmup processors caches, etc..

So for me it looks like that the mode of a fork-per-benchmark method should be the preferred default.

Any code can influence on thresholds inside runtime and that can influence on performance, if user doesn't know runtime implementation details and want to get accurate results he should use "external". We can change default, but I amn't sure that it's right for majority of users. May be we should discuss this, it was already discussed with @qurbonzoda

ilya-g · 2021-02-17T16:29:31Z

plugin/main/src/kotlinx/benchmark/gradle/NativeMultiplatformTasks.kt

+
+            // Execute benchmark
+            if (config.iterationMode == "internal") {
+                val jsonFile = createTempFile("bench", ".json").toFile()


Just a suggestion: most kotlin.io.* functions have analogs in kotlin.io.path, so you can avoid converting the result of createTempFile to File and just use the returned Path.

The problem is that kotlin.io.createTempFile is deprecated

@Deprecated( "Avoid creating temporary files in the default temp location with this function " + "due to too wide permissions on the newly created file. " + "Use kotlin.io.path.createTempFile instead or resort to java.io.File.createTempFile." ) I used one of suggested alternatives

Sure, I mean you can use the result of kotlin.io.path.createTempFile without converting it toFile() first. Though I don't insist.

May be I didn't get. What was your suggestion? Use deprecated function in order to eliminate toFile()? Or do you mean using Path, not File?

plugin/main/src/kotlinx/benchmark/gradle/BenchmarkConfiguration.kt

plugin/main/src/kotlinx/benchmark/gradle/NativeMultiplatformTasks.kt

runtime/nativeMain/src/kotlinx/benchmark/native/NativeExecutor.kt

plugin/main/src/kotlinx/benchmark/gradle/NativeMultiplatformTasks.kt

runtime/nativeMain/src/kotlinx/benchmark/native/NativeExecutor.kt

runtime/nativeMain/src/kotlinx/benchmark/Utils.kt

ilya-g · 2021-02-19T00:17:01Z

README.md

@@ -166,6 +162,8 @@ Available configuration options:
 * `iterationTimeUnit` – time unit for `iterationTime` (default is seconds)
 * `outputTimeUnit` – time unit for results output
 * `mode` – "thrpt" for measuring operations per time, or "avgt" for measuring time per operation
+* `iterationMode` – "external" for iterating in gradle in order to get correct Kotlin/Native runtime input in measurement,
+ "internal" can be used if it's known that measured code have no calls in K/N runtime that can influence on measurement unrepeatedly.


It feels conceptually like 'fork' option in JMH (see the example). Perhaps we could find a resembling name for this option.

ilya-g · 2021-02-19T00:22:55Z

README.md

@@ -166,6 +162,8 @@ Available configuration options:
 * `iterationTimeUnit` – time unit for `iterationTime` (default is seconds)
 * `outputTimeUnit` – time unit for results output
 * `mode` – "thrpt" for measuring operations per time, or "avgt" for measuring time per operation
+* `iterationMode` – "external" for iterating in gradle in order to get correct Kotlin/Native runtime input in measurement,
+ "internal" can be used if it's known that measured code have no calls in K/N runtime that can influence on measurement unrepeatedly.


singleton initialization

I always thought that microbenchmarks tried to measure the settled code execution performance, i.e. when all fixed initialization costs are done and no longer affect the performance of the code being measured. That's why they do warm-up iterations.
So for me it looks like that the mode of a fork-per-benchmark method should be the preferred default.

qwwdfsad · 2021-03-02T14:51:35Z

runtime/nativeMain/src/kotlinx/benchmark/NativeBlackhole.kt

+    actual fun consume(obj: Any?) {
+        // hashCode now is implemented as taking address of object, so it's suitable now.
+        // If implementation is changed `Blackhole` should be reimplemented.
+        consumer += obj.hashCode()


That's a virtual call that also may be way too heavy for a user-supplied class. That's just a time bomb waiting to explode, so I'd suggest using Blackhole approach (non-ordered volatile write, LLVM directive, w/e).

Also, see https://bugs.openjdk.java.net/browse/JDK-8252505 for the potential pitfalls

qwwdfsad · 2021-03-02T14:52:51Z

runtime/nativeMain/src/kotlinx/benchmark/NativeBlackhole.kt

+        consumer += obj.hashCode()
+    }
+    actual fun consume(bool: Boolean) {
+        consumer += bool.hashCode()


Here and below, this is still a write, moreover, it is a write to @ThreadLocal object field, so it's unlikely to be add instruction to a known address.

Let's please avoid any writes in performance sensitive-code

qwwdfsad · 2021-03-02T14:58:29Z

runtime/nativeMain/src/kotlinx/benchmark/native/NativeExecutor.kt

        val startTime = getTimeNanos()
        while (counter-- > 0) {
            @Suppress("UNUSED_VARIABLE")
            val result = instance.executeFunction() // ignore result for now, but might need to consume it somehow
        }
+        GC.collect()


Unconditional GC between each measurement iteration implies multiple things:

Small micro-nano benchmarks may be dominated by this runtime call with undesired side-effects

The amortized cost of allocations (especially TLAB-like) is simply not measured. E.g. it would be impossible to compare two JSON parsers that differentiate mostly in allocation patterns

We potentially mess up with CPU caches on each iteration. That's not really helpful for correct measurements and comparison, especially when comparing regular and cache-obvious algorithms

I strongly suggest making GC after each iteration optional and disabled by default

I got your concerns, I can do several modes. We can even use fully turned off GC as the second mode. I really don't like an idea to. run benchmark with only auto GC calls made by runtime, because then measurements can differ a lot between different runs of benchmarks. Moreover, some users expect that the result of running same benchmark as separate application should be close to results of benchmarking with library, that's impossible with auto-called GC.

What do you think if we make 2 modes: one with calling GC after each iteration (behaviour similar to separate application) and without GC calls at all?

IMO It's hard to actually have non-micro benchmarks without GC at all, but in my (JVM-ish) experience I mostly used to measure GC as amortized part of the execution. And in really rare cases (whether I want precision, a minimal amount of noise or do not care about memory pressure) I use -prof gc that collects garbage after each iteration.

I think it's ok to have three modes for GC: none, auto and iteration with auto as default

qurbonzoda · 2021-07-12T23:09:20Z

plugin/main/src/kotlinx/benchmark/gradle/NativeMultiplatformTasks.kt

@@ -114,28 +116,133 @@ fun Project.createNativeBenchmarkExecTask(
        onlyIf { linkTask.enabled }

        val reportsDir = benchmarkReportsDir(config, target)
-        val reportFile = reportsDir.resolve("${target.name}.${config.reportFileExt()}")
+        reportFile = reportsDir.resolve("${target.name}.json")


Preserving ${config.reportFileExt()} is enough to support specified report format.

qurbonzoda · 2021-07-12T23:31:49Z

plugin/main/src/kotlinx/benchmark/gradle/NativeMultiplatformTasks.kt

+        // Get full list of running benchmarks
+        execute(listOf(configFile.absolutePath, "--list", benchsDescriptionDir.absolutePath))
+        val detailedConfigFiles = project.fileTree(benchsDescriptionDir).files.sortedBy { it.absolutePath }
+        val jsonReportParts = mutableListOf<File>()


Seems it is a redundant val

qurbonzoda · 2021-07-12T23:50:25Z

runtime/nativeMain/src/kotlinx/benchmark/NativeBlackhole.kt

@@ -1,13 +1,178 @@
 package kotlinx.benchmark

+import kotlinx.cinterop.pin
+import kotlin.experimental.xor


Please comment out these imports as well

qurbonzoda · 2021-07-20T13:29:34Z

examples/kotlin-multiplatform/build.gradle

@@ -94,10 +94,12 @@ benchmark {
        fast { // --> jvmFastBenchmark
            include("Common")
            exclude("long")
-            iterations = 1
+            iterations = 10


The purpose of the fast configuration is to take a very small time. The configuration is usually run to test if the project still builds and runs benchmarks, without emphasis on correctness. Maybe 5 iterations are enough?

qurbonzoda · 2021-07-20T14:57:20Z

plugin/main/src/kotlinx/benchmark/gradle/BenchmarkConfiguration.kt

@@ -9,6 +9,8 @@ open class BenchmarkConfiguration(val extension: BenchmarksExtension, val name:
    var iterationTime: Long? = null
    var iterationTimeUnit: String? = null
    var mode: String? = null
+    var nativeIterationMode: String? = null // TODO: where should warning about K/N specific of this parameter be shown?


Can these advanced platform-specific options be defined using advanced(name, value)?

qurbonzoda · 2021-07-20T16:59:33Z

runtime/nativeMain/src/kotlinx/benchmark/native/NativeExecutor.kt

    ): Double {
        val executeFunction = benchmark.function
        var counter = cycles
+        GC.collect()


Is GC.collect() before each iteration intentional even if nativeGCCollectMode is auto?

warmup also does GC.collect()

qurbonzoda · 2021-08-06T00:18:04Z

plugin/main/src/kotlinx/benchmark/gradle/BenchmarkConfiguration.kt

@@ -9,6 +9,7 @@ open class BenchmarkConfiguration(val extension: BenchmarksExtension, val name:
    var iterationTime: Long? = null
    var iterationTimeUnit: String? = null
    var mode: String? = null
+    var nativeGCCollectMode: String? = null


I would rather make nativeGCCollectMode be declared via advanced(...) as well, similar to nativeIterationMode.

ilya-g · 2021-08-06T09:09:55Z

Several discussions are left unresolved here, and I think the merge was too early.
We should resolve outstanding questions with follow-up commits/PRs to the master.

LepilkinaElena · 2021-08-06T13:09:16Z

It was discussed with @qurbonzoda in direct messages and it's decided to merge it, I got an approve to merge from him.

@ilya-g could you please write all your concerns.

LepilkinaElena requested a review from qurbonzoda February 15, 2021 14:03

StefMa reviewed Feb 15, 2021

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

plugin/build.gradle Outdated Show resolved Hide resolved

runtime/build.gradle Outdated Show resolved Hide resolved

runtime/build.gradle Outdated Show resolved Hide resolved

qwwdfsad self-requested a review February 15, 2021 15:04

ilya-g reviewed Feb 17, 2021

View reviewed changes

qurbonzoda reviewed Feb 18, 2021

View reviewed changes

ilya-g reviewed Feb 19, 2021

View reviewed changes

whyoleg mentioned this pull request Feb 20, 2021

Multi-format reports #35

Merged

qwwdfsad requested changes Mar 2, 2021

View reviewed changes

LepilkinaElena force-pushed the lepilkina/native_support branch from 53e9cc3 to da7e08a Compare April 10, 2021 08:43

LepilkinaElena requested review from qwwdfsad and qurbonzoda April 23, 2021 11:22

qurbonzoda requested changes Jul 20, 2021

View reviewed changes

Elena Lepilkina added 12 commits August 5, 2021 11:46

Rewrote deprecated things and add consistency with compiler flags

9e5706e

Update gradle version

85b3cb4

Added iteration mode to configuration

2c15f05

Support K/N runs

e91d15c

Removed information from README

c4c9da9

Renamed several variables

eaf4a71

Fixed internal mode after rebase

8b94c00

Fixed extrenal benchmarks run mode

e5be4f5

Added nativeGCCollectMode setting to choose strategy of calling K/N GC

a1661c2

Changed Blackhole implementation

f3c306f

Review fixes

e2b2d91

Made nativeIterationMode advanced option

0da9663

LepilkinaElena force-pushed the lepilkina/native_support branch from da7e08a to 0da9663 Compare August 5, 2021 09:36

qurbonzoda approved these changes Aug 6, 2021

View reviewed changes

Made nativeGCCollectMode advanced configuration parameter

ce8e274

LepilkinaElena merged commit 43884af into master Aug 6, 2021

LepilkinaElena deleted the lepilkina/native_support branch August 6, 2021 08:33

qurbonzoda mentioned this pull request Jan 12, 2022

Native benchmarks runner doesn't allow to get right measurements #24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kotlin/Native support #32

Kotlin/Native support #32

LepilkinaElena commented Feb 15, 2021

ilya-g Feb 17, 2021

LepilkinaElena Feb 18, 2021 •

edited

LepilkinaElena Feb 18, 2021

ilya-g Feb 19, 2021

ilya-g Feb 19, 2021

LepilkinaElena Feb 19, 2021

LepilkinaElena Feb 19, 2021

ilya-g Feb 17, 2021

LepilkinaElena Feb 18, 2021

ilya-g Feb 18, 2021

LepilkinaElena Feb 19, 2021

ilya-g Feb 19, 2021

ilya-g Feb 19, 2021

qwwdfsad Mar 2, 2021

qwwdfsad Mar 2, 2021

qwwdfsad Mar 2, 2021

LepilkinaElena Apr 6, 2021

qwwdfsad Apr 6, 2021

qurbonzoda Jul 12, 2021

qurbonzoda Jul 12, 2021

qurbonzoda Jul 12, 2021

qurbonzoda Jul 20, 2021

qurbonzoda Jul 20, 2021

qurbonzoda Jul 20, 2021

qurbonzoda Jul 20, 2021

qurbonzoda Aug 6, 2021

ilya-g commented Aug 6, 2021

LepilkinaElena commented Aug 6, 2021

Kotlin/Native support #32

Kotlin/Native support #32

Conversation

LepilkinaElena commented Feb 15, 2021

Choose a reason for hiding this comment

LepilkinaElena Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilya-g commented Aug 6, 2021

LepilkinaElena commented Aug 6, 2021

LepilkinaElena Feb 18, 2021 •

edited