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

Benchmarks. Mimic the behavior of Skiko rendering #5215

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@ import androidx.compose.runtime.Composable
import androidx.compose.ui.InternalComposeUiApi
import androidx.compose.ui.graphics.asComposeCanvas
import androidx.compose.ui.scene.CanvasLayersComposeScene
import androidx.compose.ui.scene.ComposeScene
import androidx.compose.ui.unit.IntSize
import org.jetbrains.skia.Surface
import kotlin.time.Duration
import kotlin.time.Duration.Companion.nanoseconds
import kotlin.time.ExperimentalTime
import kotlinx.coroutines.*
import org.jetbrains.skia.Color
import org.jetbrains.skia.PictureRecorder
import org.jetbrains.skia.Rect
import kotlin.time.TimeSource.Monotonic.markNow
import kotlin.time.measureTime

Expand Down Expand Up @@ -46,16 +50,15 @@ suspend fun measureComposable(
graphicsContext: GraphicsContext?,
content: @Composable () -> Unit
): BenchmarkResult {
val surface = graphicsContext?.surface(width, height) ?: Surface.makeNull(width, height)
val scene = CanvasLayersComposeScene(size = IntSize(width, height))
try {
val nanosPerFrame = (1.0 / targetFps.toDouble() * nanosPerSecond).toLong()
scene.setContent(content)
val surface = graphicsContext?.surface(width, height) ?: Surface.makeNull(width, height)
val canvas = surface.canvas.asComposeCanvas()

// warmup
repeat(warmupCount) {
scene.render(canvas, it * nanosPerFrame)
scene.mimicSkikoRender(surface, it * nanosPerFrame, width, height)
surface.flushAndSubmit(false)
graphicsContext?.awaitGPUCompletion()
}
Expand All @@ -67,7 +70,7 @@ suspend fun measureComposable(
if (Args.isModeEnabled(Mode.CPU)) {
renderTime = measureTime {
repeat(frameCount) {
scene.render(canvas, it * nanosPerFrame)
scene.mimicSkikoRender(surface, it * nanosPerFrame, width, height)
surface.flushAndSubmit(false)
gpuTime += measureTime {
graphicsContext?.awaitGPUCompletion()
Expand All @@ -93,7 +96,7 @@ suspend fun measureComposable(
repeat(frameCount) {
val frameStart = start + nextVSync

scene.render(canvas, nextVSync.inWholeNanoseconds)
scene.mimicSkikoRender(surface, it * nextVSync.inWholeNanoseconds, width, height)
surface.flushAndSubmit(false)

val cpuTime = frameStart.elapsedNow()
Expand Down Expand Up @@ -124,6 +127,33 @@ suspend fun measureComposable(
)
} finally {
scene.close()
surface.close()
runGC() // cleanup for next benchmarks
}
}

private val pictureRecorder = PictureRecorder()

/**
* Mimic Skiko render logic from https://github.com/JetBrains/skiko/blob/eb1f04ec99d50ff0bdb2f592fdf49711a9251aa7/skiko/src/awtMain/kotlin/org/jetbrains/skiko/SkiaLayer.awt.kt#L531
*
* This is very simplified logic, and it still can differ from the real cases.
*
* Though one main function - rendering into picture - was affecting performance.
*
* Benchmarks showed an improvement by 10%, but there was a regression by 10%.
*
* Beware that this logic can be changed in some new version of Skiko.
*
* If Skiko stops using `picture`, we need to remove it here too.
*/
@OptIn(InternalComposeUiApi::class)
fun ComposeScene.mimicSkikoRender(surface: Surface, time: Long, width: Int, height: Int) {
Copy link
Member

Choose a reason for hiding this comment

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

imo: requires BIG TODO to keep it in sync with skiko implementation, because I'm not sure that it won't be changed there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment here

Without reusing the whole renderer from Skiko (which realistically we won't do without a good reason), we should keep it sync manually.

When we merge it, I will add a comment to Skiko:

Note, if you change the rendering logic a lot, notify about this in [this issue](new created issue), because [benchmarks project](link to this function) depends on it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rename it to reflect what it actually does , f.i renderViaPicture.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function should match Skiko’s rendering.

While it currently uses a picture, that’s an implementation detail. If Skiko stops using a picture, we should change the implementation here, without renaming.

It also clears the Canvas, which may impact benchmarking accuracy.

Ideally we should reuse some function from Skiko, but this a much more complex task.

val pictureCanvas = pictureRecorder.beginRecording(Rect(0f, 0f, width.toFloat(), height.toFloat()))
render(pictureCanvas.asComposeCanvas(), time)
val picture = pictureRecorder.finishRecordingAsPicture()

surface.canvas.clear(Color.TRANSPARENT)
surface.canvas.drawPicture(picture)
picture.close()
}
1 change: 1 addition & 0 deletions benchmarks/multiplatform/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ allprojects {
google()
mavenCentral()
maven("https://maven.pkg.jetbrains.space/public/p/compose/dev")
mavenLocal()
}
}