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

Build Skiko with Clang-CL #1020

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

Conversation

Alovchin91
Copy link
Contributor

@Alovchin91 Alovchin91 commented Feb 3, 2025

Official Skia documentation suggests that it's highly recommended to build Skia with Clang-CL on Windows, and that this dramatically improves Skia performance with Software rendering and in other areas. This corresponds with my experience, so here it is.

Related Skia-pack PR: JetBrains/skia-pack#64

There are a couple more changes in this PR:

  • It removes the BreakIterator.clone() method as it's broken on JVM, seemingly due to a "use-after-free" bug.

  • It changes the build-with-local-skia.sh a bit to correctly determine the ARM64 platform. There is currently no native MSYS2 for ARM64, and the emulated one will return x86_64 for uname. So we should use Powershell instead.

@Alovchin91 Alovchin91 marked this pull request as ready for review February 4, 2025 18:11
@Alovchin91
Copy link
Contributor Author

The CI is failing because this PR depends on a new Skia-pack release (-2).

@igordmn igordmn self-requested a review February 4, 2025 18:40
Comment on lines -308 to -315
/**
* Create a copy of this iterator
*/
fun clone(): BreakIterator {
Stats.onNativeCall()
return BreakIterator(withErrorGuard("Failed to clone") { _nClone(_ptr, it) })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reckon this change would probably require bumping Skiko version to 0.9 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, we'll build the next version as 0.9.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Explanation why it is deleted in an internal channel: https://jetbrains.slack.com/archives/C02DDNREC77/p1738678000232729

  • breakIteratorCloneTest is failing with an access violation in Debug mode with additional checks, so using it wasn't safe
  • it is not used anywhery

skiko/buildSrc/src/main/kotlin/properties.kt Outdated Show resolved Hide resolved
Comment on lines -308 to -315
/**
* Create a copy of this iterator
*/
fun clone(): BreakIterator {
Stats.onNativeCall()
return BreakIterator(withErrorGuard("Failed to clone") { _nClone(_ptr, it) })
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, we'll build the next version as 0.9.0

@igordmn
Copy link
Collaborator

igordmn commented Feb 7, 2025

Clang gives a big increase in performance (~ 16% FPS from 2 runs on average) on Windows x64 / DirectX. Testing to post more precise results with different conditions. Software rendering will be more faster.

@igordmn
Copy link
Collaborator

igordmn commented Feb 10, 2025

Benchmarks results

Binary size 27 % is smaller
DirectX rendering FPS is 17 % higher
Software rendering FPS is 73 % higher

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