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

Improve configuration of native image compilation #509

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

bioball
Copy link
Member

@bioball bioball commented May 29, 2024

This is supercedes and closes #455

Thanks to @translatenix for doing 99% of the work here

  • Remove unnecessary --add-opens by deferring initialization of MessagePack library at build time
  • Use quick build mode for 40% faster compilation and 20% smaller executable when not creating release builds
  • Add test for native executables when running in server mode

@bioball bioball requested review from stackoverflow and holzensp May 30, 2024 14:58
Copy link
Contributor

@stackoverflow stackoverflow left a comment

Choose a reason for hiding this comment

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

Looks good. Just some minor comments.

.withPathSensitivity(PathSensitivity.RELATIVE)
}

private fun Test.configureExecutableTest(engineName: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why testWindowsExecutableAmd64 doesn't call this function, just mac and linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

}

private val transports: Pair<MessageTransport, MessageTransport> = run {
if (USE_DIRECT_TRANSPORT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is always false, why have an if?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just here to allow us to test the same logic if we wanted to run the server tests with an encoder/decoder. It's for development purposes. I'll add a note here.

odenix and others added 4 commits June 3, 2024 10:31
- Solve msgpack issue with `--initialize-at-run-time`.
- Use quick build mode for non-release builds:
  40% faster compilation, 20% smaller executable.
- Remove options that were commented out.
Motivation:
- improve test coverage of server mode
- verify that "--initialize-at-run-time=org.msgpack.core.buffer.DirectBufferAccess"
  works fine and opening JDK internals with "--add-opens" isn't required

Changes:
- split ServerTest into JvmServerTest and NativeServerTest
- extract native executable paths to PklExecutablePaths
Comment on lines 25 to 27
companion object {
private const val USE_DIRECT_TRANSPORT = false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the reasons of using a companion object instead of a normal constant in that kt file?

Copy link
Member Author

@bioball bioball Jun 3, 2024

Choose a reason for hiding this comment

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

You mean, why not move this up to the file level? I tend to prefer this approach because it more easily translates to Java (you don't end up with a synthetic FooKt class).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes exactly, that was my question 🙃.
But actually you end up with a Foo.Companion 🤷‍♂️ 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, true. We already need the companion class, and we could mark them @JvmStatic to get around it. But, I think this would be splitting hairs a little bit.

@bioball bioball merged commit d81a123 into apple:main Jun 4, 2024
5 checks passed
@bioball bioball deleted the graalvm-23 branch June 4, 2024 00:08
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.

4 participants