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. Support GPU on Windows #5216

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Jan 27, 2025

Release notes

N/A

@@ -1,4 +1,4 @@
compose.version=1.7.1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We shouldn't merge until we build compose with JetBrains/skiko#1016.

The PR only works with custom Skiko right now:

// in skiko
checkout igor.demin/DirectXOffscreenContext
./gradlew publishToMavenLocal -deploy.version=722.0.0

// in benchmarks in build.gradle
    val desktopMain by getting {  
            dependencies {
                ...
                implementation("org.jetbrains.skiko:skiko:722.0.0-SNAPSHOT")  
                implementation("org.jetbrains.skiko:skiko-awt-runtime-macos-arm64:722.0.0-SNAPSHOT")  

Copy link
Collaborator Author

@igordmn igordmn Feb 7, 2025

Choose a reason for hiding this comment

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

Changed.

Though, after migrating to new Compose, we can't no longer test pre 1.8.0+dev2047 version without manually commenting the not-compiling DirectXOffscreenContext/DirectXOffscreenContext.

Is it fine, or should we wait merging?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is better to make the dependency reflective for now?

However this way, we cannot compare with the old version anyway. I would add an option to choose between null/GPU/software rendering in addition, with backward compatible defaults.

I would probably also wait GPU renderers for non-Windows desktop targets to have common code here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would add an option

Because we need to include/exclude compilation of new classes, we have to use a Gradle property to include/exclude additional sourceset/module. It works, but complicates the build and structure. A simpler solution is to wait merging.

What, if we wait for 1.8.0 Stable? We always could back in time, and checkout v1.8.0 tag for old versions testing.

I would probably also wait GPU renderers for non-Windows desktop targets to have common code here.

Let's discuss again at the 1.8.0’s release. I would prefer merging it even if only Windows supported at that moment, as it seems merging only it doesn't have downsides?

@igordmn igordmn requested a review from pjBooms January 27, 2025 03:28
@igordmn igordmn changed the title Support GPU on Windows Benchmarks. Support GPU on Windows Jan 27, 2025
Currently, we include the time when we wait for vsync which gives us wrong results
@igordmn igordmn force-pushed the igor.demin/gpu-support branch from 5052874 to 9815867 Compare January 27, 2025 14:55
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