ronantakizawa/codereview-bench huggingface dataset:
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceReader.kt (Kotlin)

Code before:

/**
 * Reads the content of the resource file at the specified path and returns it as a byte array.
 *
 * @param path The path of the file to read in the resource's directory.
 * @return The content of the file as a byte array.
 */
@InternalResourceApi
suspend fun readResourceBytes(path: String): ByteArray = getDefaultResourceReader().read(path)

/**
 * Provides the platform dependent URI for a given resource path.
 *
 * @param path The path to the file in the resource's directory.
 * @return The URI string of the specified resource.
 */
@InternalResourceApi
fun getResourceUri(path: String): String = getDefaultResourceReader().getUri(path)

interface ResourceReader {
    suspend fun read(path: String): ByteArray
    suspend fun readPart(path: String, offset: Long, size: Long): ByteArray
    fun getUri(path: String): String
}

expect fun getDefaultResourceReader(): ResourceReader

//ResourceReader provider will be overridden for tests
val LocalResourceReader =
    staticCompositionLocalOf { getDefaultResourceReader() }

//For an android preview we need to initialize the resource reader with the local context
internal expect val ProvidableCompositionLocal<ResourceReader>.currentOrPreview: ResourceReader
    @Composable get


Review comment (suggestion):
Consider making `val DefaultResourceReader` to be consistent with the platform implementations

Code after:

/**
 * Reads the content of the resource file at the specified path and returns it as a byte array.
 *
 * @param path The path of the file to read in the resource's directory.
 * @return The content of the file as a byte array.
 */
@InternalResourceApi
suspend fun readResourceBytes(path: String): ByteArray = DefaultResourceReader.read(path)

/**
 * Provides the platform dependent URI for a given resource path.
 *
 * @param path The path to the file in the resource's directory.
 * @return The URI string of the specified resource.
 */
@InternalResourceApi
fun getResourceUri(path: String): String = DefaultResourceReader.getUri(path)

@ExperimentalResourceApi
interface ResourceReader {
    suspend fun read(path: String): ByteArray
    suspend fun readPart(path: String, offset: Long, size: Long): ByteArray
    fun getUri(path: String): String
}

internal expect fun getPlatformResourceReader(): ResourceReader

@ExperimentalResourceApi
internal val DefaultResourceReader = getPlatformResourceReader()

//ResourceReader provider will be overridden for tests
@ExperimentalResourceApi
val LocalResourceReader = staticCompositionLocalOf { DefaultResourceReader }

//For an android preview we need to initialize the resource reader with the local context
internal expect val ProvidableCompositionLocal<ResourceReader>.currentOrPreview: ResourceReader
    @Composable get
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceReader.kt (Kotlin)

Code before:
 * Reads the content of the resource file at the specified path and returns it as a byte array.
 *
 * @param path The path of the file to read in the resource's directory.
 * @return The content of the file as a byte array.
 */
@InternalResourceApi
suspend fun readResourceBytes(path: String): ByteArray = getDefaultResourceReader().read(path)

/**
 * Provides the platform dependent URI for a given resource path.
 *
 * @param path The path to the file in the resource's directory.
 * @return The URI string of the specified resource.
 */
@InternalResourceApi
fun getResourceUri(path: String): String = getDefaultResourceReader().getUri(path)

@ExperimentalResourceApi
interface ResourceReader {
    suspend fun read(path: String): ByteArray
    suspend fun readPart(path: String, offset: Long, size: Long): ByteArray
    fun getUri(path: String): String
}

@ExperimentalResourceApi
expect fun getDefaultResourceReader(): ResourceReader

//ResourceReader provider will be overridden for tests
@ExperimentalResourceApi
val LocalResourceReader =
    staticCompositionLocalOf { getDefaultResourceReader() }

//For an android preview we need to initialize the resource reader with the local context
internal expect val ProvidableCompositionLocal<ResourceReader>.currentOrPreview: ResourceReader
    @Composable get


Review comment (refactor):
since `JvmResourceReader.Default` is object, why did you refactored this code?

Code after:
 * Reads the content of the resource file at the specified path and returns it as a byte array.
 *
 * @param path The path of the file to read in the resource's directory.
 * @return The content of the file as a byte array.
 */
@InternalResourceApi
suspend fun readResourceBytes(path: String): ByteArray = DefaultResourceReader.read(path)

/**
 * Provides the platform dependent URI for a given resource path.
 *
 * @param path The path to the file in the resource's directory.
 * @return The URI string of the specified resource.
 */
@InternalResourceApi
fun getResourceUri(path: String): String = DefaultResourceReader.getUri(path)

@ExperimentalResourceApi
interface ResourceReader {
    suspend fun read(path: String): ByteArray
    suspend fun readPart(path: String, offset: Long, size: Long): ByteArray
    fun getUri(path: String): String
}

internal expect fun getPlatformResourceReader(): ResourceReader

@ExperimentalResourceApi
internal val DefaultResourceReader = getPlatformResourceReader()

//ResourceReader provider will be overridden for tests
@ExperimentalResourceApi
val LocalResourceReader = staticCompositionLocalOf { DefaultResourceReader }

//For an android preview we need to initialize the resource reader with the local context
internal expect val ProvidableCompositionLocal<ResourceReader>.currentOrPreview: ResourceReader
    @Composable get
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceReader.desktop.kt (Kotlin)

Code before:
package org.jetbrains.compose.resources

import java.io.InputStream

@ExperimentalResourceApi
internal actual fun getPlatformResourceReader(): ResourceReader =
    JvmResourceReader.Default

@ExperimentalResourceApi
class JvmResourceReader(
    private val classLoader: ClassLoader = JvmResourceReader::class.java.classLoader
) : ResourceReader {

    companion object {
        val Default = JvmResourceReader()
    }

    override suspend fun read(path: String): ByteArray {
        val resource = getResourceAsStream(path)
        return resource.use { input -> input.readBytes() }
    }

    override suspend fun readPart(path: String, offset: Long, size: Long): ByteArray {
        val resource = getResourceAsStream(path)
        val result = ByteArray(size.toInt())
        resource.use { input ->
            input.skipBytes(offset)
            input.readNBytes(result, 0, size.toInt())
        }
        return result
    }

    //skipNBytes requires API 12
    private fun InputStream.skipBytes(offset: Long) {
        var skippedBytes = 0L
        while (skippedBytes < offset) {
            val count = skip(offset - skippedBytes)
            if (count == 0L) break
            skippedBytes += count
        }
    }


Review comment (suggestion):
I guess it is better
```kotlin
@ExperimentalResourceApi
internal actual fun getPlatformResourceReader(): ResourceReader = JvmResourceReader()

@ExperimentalResourceApi 
class JvmResourceReader(
    private val classLoader: ClassLoader = JvmResourceReader::class.java.classLoader
) : ResourceReader {
  //no companion object
}
```

Code after:
package org.jetbrains.compose.resources

import java.io.InputStream

@ExperimentalResourceApi
internal actual fun getPlatformResourceReader(): ResourceReader =
    JvmResourceReader.Default

@ExperimentalResourceApi
class JvmResourceReader(
    private val classLoader: ClassLoader
) : ResourceReader {

    companion object {
        internal val Default = JvmResourceReader(JvmResourceReader::class.java.classLoader)
    }

    override suspend fun read(path: String): ByteArray {
        val resource = getResourceAsStream(path)
        return resource.use { input -> input.readBytes() }
    }

    override suspend fun readPart(path: String, offset: Long, size: Long): ByteArray {
        val resource = getResourceAsStream(path)
        val result = ByteArray(size.toInt())
        resource.use { input ->
            input.skipBytes(offset)
            input.readNBytes(result, 0, size.toInt())
        }
        return result
    }

    //skipNBytes requires API 12
    private fun InputStream.skipBytes(offset: Long) {
        var skippedBytes = 0L
        while (skippedBytes < offset) {
            val count = skip(offset - skippedBytes)
            if (count == 0L) break
            skippedBytes += count
        }
    }
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceCaches.kt (Kotlin)

Code before:
package org.jetbrains.compose.resources

private val caches = mutableListOf<AsyncCache<Any, Any>>()

internal fun registerCache(cache: AsyncCache<Any, Any>) {
    caches.add(cache)
}

/**
 * Clears any cached resources maintained internally by the system.
 *
 * This method is intended for use from Compose Hot Reload,
 * to reload resources that might be changed.
 * It can be also useful to release memory or reset cached resources that
 * may no longer be required.
 *
 * Note that frequent or unnecessary calls to this function may impact
 * performance by removing resources that might otherwise benefit from being cached.
 */
fun dropResourceCaches() {
   caches.forEach { it.clear() }
}

Review comment (performance):
```kotlin
private val caches = mutableListOf<AsyncCache<*, *>>()

internal fun registerCache(cache: AsyncCache<*, *>) {
    caches.add(cache)
}
```

Code after:
package org.jetbrains.compose.resources

import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock

internal class AsyncCache<K, V> {
    private val mutex = Mutex()
    private val cache = mutableMapOf<K, Deferred<V>>()

    init {
        ResourceCaches.registerCache(this)
    }

    suspend fun getOrLoad(key: K, load: suspend () -> V): V = coroutineScope {
        val deferred = mutex.withLock {
            var cached = cache[key]
            if (cached == null || cached.isCancelled) {
                //LAZY - to free the mutex lock as fast as possible
                cached = async(start = CoroutineStart.LAZY) { load() }
                cache[key] = cached
            }
            cached
        }
        deferred.await()
    }

    suspend fun clear() {
        mutex.withLock {
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceCaches.kt (Kotlin)

Code before:
package org.jetbrains.compose.resources

private val caches = mutableListOf<AsyncCache<*, *>>()

internal fun registerCache(cache: AsyncCache<*, *>) {
    caches.add(cache)
}

/**
 * Clears any cached resources maintained internally by the system.
 *
 * It can be useful to release memory or reset cached resources that
 * may be changed or no longer be required.
 *
 * Note that frequent or unnecessary calls to this function may impact
 * performance by removing resources that might otherwise benefit from being cached.
 */
fun dropResourceCaches() {
   caches.forEach { it.clear() }
}

Review comment (performance):
Let's create `ResourceCaches` object with this, and move the code into AsyncCache file. (the top level function is too wide visible)

```kotlin
internal object ResourceCaches {
  private val caches = mutableListOf<AsyncCache<*, *>>()

/**
 * Clears any cached resources maintained internally by the system.
 *
 * It can be useful to release memory or reset cached resources that
 * may be changed or no longer be required.
 *
 * Note that frequent or unnecessary calls to this function may impact
 * performance by removing resources that might otherwise benefit from being cached.
 */
  fun drop() {
     caches.forEach { it.clear() }
  }
}
```

And the `registerCache` function will not be necessary then.

Code after:
package org.jetbrains.compose.resources

import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock

internal class AsyncCache<K, V> {
    private val mutex = Mutex()
    private val cache = mutableMapOf<K, Deferred<V>>()

    init {
        ResourceCaches.registerCache(this)
    }

    suspend fun getOrLoad(key: K, load: suspend () -> V): V = coroutineScope {
        val deferred = mutex.withLock {
            var cached = cache[key]
            if (cached == null || cached.isCancelled) {
                //LAZY - to free the mutex lock as fast as possible
                cached = async(start = CoroutineStart.LAZY) { load() }
                cache[key] = cached
            }
            cached
        }
        deferred.await()
    }

    suspend fun clear() {
        mutex.withLock {
            cache.clear()
        }
    }
}

object ResourceCaches {
    private val caches = mutableListOf<AsyncCache<*, *>>()

    internal fun registerCache(cache: AsyncCache<*, *>) = caches.add(cache)

    /**
     * Clears any cached resources maintained internally by the system.
     *
Repository: JetBrains/compose-multiplatform
File: components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceCaches.kt (Kotlin)

Code before:
                cache[key] = cached
            }
            cached
        }
        deferred.await()
    }

    suspend fun clear() {
        mutex.withLock {
            cache.clear()
        }
    }
}

/**
 * Manages internal resource caches.
 *
 * While the caches are managed automatically, there might be scenarios where manual cache clearing
 * is desired. For such cases, we provide an API to reset the caches.
*/
object ResourceCaches {
    private val caches = mutableListOf<AsyncCache<*, *>>()

    internal fun registerCache(cache: AsyncCache<*, *>) = caches.add(cache)

    internal suspend fun asyncClear() {
        caches.forEach { it.clear() }
    }
}


Review comment (suggestion):
let's rename it to `internal suspend fun clear()` and move the doc to the function

Code after:
                cache[key] = cached
            }
            cached
        }
        deferred.await()
    }

    suspend fun clear() {
        mutex.withLock {
            cache.clear()
        }
    }
}

object ResourceCaches {
    private val caches = mutableListOf<AsyncCache<*, *>>()

    internal fun registerCache(cache: AsyncCache<*, *>) = caches.add(cache)

    /**
     * Clears any cached resources maintained internally by the system.
     *
     * It can be useful to release memory or reset cached resources that
     * may be changed or no longer be required.
     */
    internal suspend fun clear() {
        caches.forEach { it.clear() }
    }
}
Repository: JetBrains/compose-multiplatform
File: gradle-plugins/compose/src/test/test-projects/misc/androidAppWithResources/featureModule/build.gradle.kts (Kotlin)

Code before:
plugins {
    id("org.jetbrains.compose")
    kotlin("multiplatform")
    kotlin("plugin.compose")
    id("com.android.kotlin.multiplatform.library")
}

kotlin {
    jvmToolchain(11)
    jvm()

    androidLibrary {
        experimentalProperties["android.experimental.kmp.enableAndroidResources"] = true
        namespace = "me.sample.feature"
        compileSdk = 35
        minSdk = 23
    }

    sourceSets {
        commonMain.dependencies {
            api("org.jetbrains.compose.runtime:runtime:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.material:material:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.components:components-resources:COMPOSE_VERSION_PLACEHOLDER")
        }
    }
}

//https://youtrack.jetbrains.com/issue/CMP-8325
compose.desktop {
    application { }
}

Review comment (suggestion):
It requires JDK 11 to be installed, but it's not the case in our pipeline anymore.
It might be replaced like [this](https://github.com/JetBrains/compose-multiplatform/pull/5424/commits/51a87ca55cd5bc7e8b9cc78b6c7c1531bda2d96a) if you need to specify compatibility, or just drop it if it doesn't matter

Code after:
plugins {
    id("org.jetbrains.compose")
    kotlin("multiplatform")
    kotlin("plugin.compose")
    id("com.android.kotlin.multiplatform.library")
}

kotlin {
    jvm()

    androidLibrary {
        namespace = "me.sample.feature"
        compileSdk = 35
        minSdk = 23
        androidResources.enable = true
    }

    sourceSets {
        commonMain.dependencies {
            api("org.jetbrains.compose.runtime:runtime:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.material:material:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.components:components-resources:COMPOSE_VERSION_PLACEHOLDER")
        }
    }
}

//https://youtrack.jetbrains.com/issue/CMP-8325
compose.desktop {
    application { }
}

compose.resources {
    publicResClass = true
}
Repository: JetBrains/compose-multiplatform
File: gradle-plugins/compose/src/test/test-projects/misc/androidAppWithResources/featureModule/build.gradle.kts (Kotlin)

Code before:
plugins {
    id("org.jetbrains.compose")
    kotlin("multiplatform")
    kotlin("plugin.compose")
    id("com.android.kotlin.multiplatform.library")
}

kotlin {
    jvmToolchain(11)
    jvm()

    androidLibrary {
        experimentalProperties["android.experimental.kmp.enableAndroidResources"] = true
        namespace = "me.sample.feature"
        compileSdk = 35
        minSdk = 23
    }

    sourceSets {
        commonMain.dependencies {
            api("org.jetbrains.compose.runtime:runtime:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.material:material:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.components:components-resources:COMPOSE_VERSION_PLACEHOLDER")
        }
    }
}

//https://youtrack.jetbrains.com/issue/CMP-8325
compose.desktop {
    application { }
}

Review comment (suggestion):
I guess it shouldn't be reverted to the previous version, please check conflicts/rebase and revert unrelated changes

Code after:
plugins {
    id("org.jetbrains.compose")
    kotlin("multiplatform")
    kotlin("plugin.compose")
    id("com.android.kotlin.multiplatform.library")
}

kotlin {
    jvm()

    androidLibrary {
        namespace = "me.sample.feature"
        compileSdk = 35
        minSdk = 23
        androidResources.enable = true
    }

    sourceSets {
        commonMain.dependencies {
            api("org.jetbrains.compose.runtime:runtime:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.material:material:COMPOSE_VERSION_PLACEHOLDER")
            api("org.jetbrains.compose.components:components-resources:COMPOSE_VERSION_PLACEHOLDER")
        }
    }
}

//https://youtrack.jetbrains.com/issue/CMP-8325
compose.desktop {
    application { }
}

compose.resources {
    publicResClass = true
}
Repository: JetBrains/compose-multiplatform
File: benchmarks/multiplatform/settings.gradle.kts (Kotlin)

Code before:
pluginManagement {
    repositories {
        mavenLocal()
        mavenCentral()
        gradlePluginPortal()
        maven("https://maven.pkg.jetbrains.space/public/p/compose/dev")
        google()
    }
}

rootProject.name = "compose-benchmarks"

dependencyResolutionManagement {
    versionCatalogs{
        create("libs") {
            // Override Kotlin and Compose versions with properties
            providers.run {
                with(gradleProperty("kotlin.version")) {
                    if (isPresent) version("kotlin", get())
                }
                with(gradleProperty("compose.version")) {
                    if (isPresent) version("compose-multiplatform", get())
                }
            }
        }
    }
}

include(":benchmarks")


Review comment (suggestion):
Can we setup an URL similar to Kotlin in the future (maybe not now). Something like:
```
https://redirector.kotlinlang.org/maven/cmp-dev/
```
This repo can be used by external users to use daily builds. `https://packages.jetbrains.team` looks like an internal link. Use cases:
- users don't want to wait for 2-weakly builds to try a just committed change
- we can ask users to try some build to investigate an issue or try a fix

Code after:
pluginManagement {
    repositories {
        mavenLocal()
        mavenCentral()
        gradlePluginPortal()
        maven("https://packages.jetbrains.team/maven/p/cmp/dev")
        google()
    }
}

rootProject.name = "compose-benchmarks"

dependencyResolutionManagement {
    versionCatalogs{
        create("libs") {
            // Override Kotlin and Compose versions with properties
            providers.run {
                with(gradleProperty("kotlin.version")) {
                    if (isPresent) version("kotlin", get())
                }
                with(gradleProperty("compose.version")) {
                    if (isPresent) version("compose-multiplatform", get())
                }
            }
        }
    }
}

include(":benchmarks")
Repository: JetBrains/compose-multiplatform
File: benchmarks/multiplatform/benchmarks/src/desktopMain/kotlin/main.desktop.kt (Kotlin)

Code before:
/*
 * Copyright 2020-2021 JetBrains s.r.o. and respective authors and developers.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE.txt file.
 */

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking

fun main(args: Array<String>) {
    Config.setGlobalFromArgs(args)

    if (Config.runServer) {
        // Start the benchmark server to receive results from browsers
        BenchmarksSaveServer.start()
    } else {
        runBlocking(Dispatchers.Main) { runBenchmarks(graphicsContext = graphicsContext()) }
    }
}


Review comment (suggestion):
Is it planned to be run only manually or/and on the server as one more indicator without replacing non-real benchmarks?

Asking because running on real devices is limited by vsync. And to know improvement we should run something heavy that always doesn't reach the vsync limit.

Code after:
/*
 * Copyright 2020-2021 JetBrains s.r.o. and respective authors and developers.
 * Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE.txt file.
 */

import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.Window
import androidx.compose.ui.window.application
import androidx.compose.ui.window.rememberWindowState
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import java.awt.GraphicsEnvironment

fun main(args: Array<String>) {
    Config.setGlobalFromArgs(args)

    if (Config.runServer) {
        // Start the benchmark server to receive results from browsers
        BenchmarksSaveServer.start()
    } else if (Config.isModeEnabled(Mode.REAL)) {
        val device = GraphicsEnvironment.getLocalGraphicsEnvironment().defaultScreenDevice
        val frameRate = device.displayMode.refreshRate.takeIf { it > 0 } ?: 120

        application {
            Window(
                onCloseRequest = ::exitApplication,
                alwaysOnTop = true,
                state = rememberWindowState(
                    width = 1920.dp, height = 1080.dp
                )
            ) {
                BenchmarkRunner(getBenchmarks(), frameRate, { System.exit(0) })
            }
        }
    } else {
        runBlocking(Dispatchers.Main) { runBenchmarks(graphicsContext = graphicsContext()) }
    }
}