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

Try native image then fallback to pure java version #717

Merged
merged 9 commits into from
Jan 24, 2023

Conversation

gzm55
Copy link
Contributor

@gzm55 gzm55 commented Oct 16, 2022

The new entry script does some light weight checking to select the first available one from:

  • native image
  • pure java version

To control the fallback logic, a new environment switch MVND_CLIENT is added. Default auto enables the fallback logic, set to native or jvm to force select the native image or pure java version.

Checking of native image is mainly based on the file name, and on Linux, there is another ldd check to ensure glibc version is OK.

@gnodet
Copy link
Contributor

gnodet commented Oct 16, 2022

I like the fallback idea, though I wonder if we should move that fallback code directly into the mvnd.sh script, which would delegate to the native binary if possible, eventually renaming mvnd to mvnd-native.
Also, we may want to investigate a way to run the client on a different JVM that the daemon. We could add two mvnd environment variables, one which would point to the default client JVM, and another that would define whether we want to always use that JVM for the client, auto switch to that JVM is the environment one is that JDK 11 compatible, or never use it. The embedded maven would then only be used if the former is not defined or if the later is set to never.

@gzm55
Copy link
Contributor Author

gzm55 commented Oct 16, 2022

A more complex fallback strategy sounds OK, we could define the rules clearly. And the main entrypoint well known is now $MVND_HOME/bin/mvnd. To keep the compatibility, can we move the mvnd.sh/mvnd.cmd (with fallback function) to mvnd or mvnd.cmd, and the native binary are moved to mvnd-native/mvnd-native.exe?

@gnodet
Copy link
Contributor

gnodet commented Oct 16, 2022

A more complex fallback strategy sounds OK, we could define the rules clearly. And the main entrypoint well known is now $MVND_HOME/bin/mvnd. To keep the compatibility, can we move the mvnd.sh/mvnd.cmd (with fallback function) to mvnd or mvnd.cmd, and the native binary are moved to mvnd-native/mvnd-native.exe?

Sounds good !

@gzm55 gzm55 changed the title add script mvnd-auto to a proper mvnd Add script mvnd-auto to a proper mvnd Oct 16, 2022
@gzm55
Copy link
Contributor Author

gzm55 commented Oct 17, 2022

Also, we may want to investigate a way to run the client on a different JVM that the daemon. We could add two mvnd environment variables, one which would point to the default client JVM, and another that would define whether we want to always use that JVM for the client, auto switch to that JVM is the environment one is that JDK 11 compatible, or never use it. The embedded maven would then only be used if the former is not defined or if the later is set to never.

@gnodet do you mean add two new env: MVND_CLIENT_JAVA_HOME and MVND_CLIENT_JAVA_HOME_REQUIRE, and when the native version fails, we select like this:

  • (MVND_CLIENT_JAVA_HOME is unset) or (MVND_CLIENT_JAVA_HOME_REQUIRE=never):
    • JAVA_HOME/bin/java or java is JDK 11+: mvnd with os java
    • JAVA_HOME/bin/java or java is old JDK: embeded maven with os java
  • MVND_CLIENT_JAVA_HOME is set, and MVND_CLIENT_JAVA_HOME_REQUIRE=always
    • always run mvnd with MVND_CLIENT_JAVA_HOME/bin/java even if MVND_CLIENT_JAVA_HOME points to an old JDK
  • MVND_CLIENT_JAVA_HOME is set, and MVND_CLIENT_JAVA_HOME_REQUIRE=auto (default value)
    • MVND_CLIENT_JAVA_HOME/bin/java is JDK 11+: mvnd with MVND_CLIENT_JAVA_HOME/bin/java
    • JAVA_HOME/bin/java or java is JDK 11+: mvnd with os java
    • JAVA_HOME/bin/java or java is old JDK: embeded maven with os java

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

I'd vote for evaluating the performance impact of wrapping mvnd binary inside a shell script before merging this.

@gzm55 gzm55 changed the title Add script mvnd-auto to a proper mvnd Draft: Add script mvnd-auto to a proper mvnd Oct 18, 2022
@gzm55
Copy link
Contributor Author

gzm55 commented Oct 18, 2022

I'd vote for evaluating the performance impact of wrapping mvnd binary inside a shell script before merging this.

when change the native image names, the checking step of native can be replaced by a test -x mvnd-native-<os_arch>, this would has no performance issue. In another pr #722 , the JDK compatible checking step will be also omitted, auto fallback is done by a multi-release jar.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 18, 2022

I wonder what is your goal here, @gzm55 ? You want to have single package installable and runnable on any host of any arch and OS combination?

@gzm55
Copy link
Contributor Author

gzm55 commented Oct 19, 2022

I wonder what is your goal here, @gzm55 ? You want to have single package installable and runnable on any host of any arch and OS combination?

Goal:

  • single official way to startup mvnd without knowing the internal structure. now there is only 4 prebuild binaries which depend on hardware and glibc 2.14, and the users have to switch to another script if he fails at first time.
  • treat mvnd as a maven distribution which runs faster than or at least same as, the original maven, then the user do not need to install another maven.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 19, 2022

Goal:

  • single official way to startup mvnd without knowing the internal structure. now there is only 4 prebuild binaries which depend on hardware and glibc 2.14, and the users have to switch to another script if he fails at first time.
  • treat mvnd as a maven distribution which runs faster than or at least same as, the original maven, then the user do not need to install another maven.

I'd be fine with all of that as long as installing the native binary as the entry point stays possible. For performance reasons package managers like SDKMAN should still prefer doing that if a native client is available for the given platform. What do others think?

@gzm55
Copy link
Contributor Author

gzm55 commented Oct 19, 2022

I'd be fine with all of that as long as installing the native binary as the entry point stays possible. For performance reasons package managers like SDKMAN should still prefer doing that if a native client is available for the given platform. What do others think?

we can use a env flag to force the entry script always use the native binary

@ppalaga
Copy link
Contributor

ppalaga commented Oct 20, 2022

Hm... am I the only one thinking that the mvnd native executable should be the entry point (rather than a wrapper shell script) on platforms for which we have mvnd native executable? @gnodet @hboutemy @cstamas @michael-o what's your opinion?

@gzm55 gzm55 changed the title Draft: Add script mvnd-auto to a proper mvnd Draft: Try native image then fallback to pure java version Oct 20, 2022
@gzm55 gzm55 changed the title Draft: Try native image then fallback to pure java version Try native image then fallback to pure java version Oct 21, 2022
@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2022

These changes seem to be addressing what I'd call install time concerns. Whether the given build of mvnd native executable can run on the current machine can be decided once at install time. Why do we need to run all those checks anew on every invocation of mvnd?

@gnodet
Copy link
Contributor

gnodet commented Oct 21, 2022

These changes seem to be addressing what I'd call install time concerns. Whether the given build of mvnd native executable can run on the current machine can be decided once at install time. Why do we need to run all those checks anew on every invocation of mvnd?

Because we don't have any installer... ? In particular, people often just unzip the tarball / zip.
https://github.com/apache/camel/blob/main/.github/actions/install-mvnd/action.yml
An alternative would be to provide an install script from a known location that would download the latest version, unzip and do the checks maybe ...

@gzm55
Copy link
Contributor Author

gzm55 commented Oct 21, 2022

These changes seem to be addressing what I'd call install time concerns. Whether the given build of mvnd native executable can run on the current machine can be decided once at install time. Why do we need to run all those checks anew on every invocation of mvnd?

how about we do a persist optimization at the real installing time for each package manager? almost every package manager has a post installing step, in which we can check via mvnd-native --status, if passed, then mv -f mvnd-native mvnd to overwrite the generic entry script.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2022

I think both proposals boil down to having a reusable piece of shell/PowerShell for linking/moving mvnd to the right file. I like that idea. @gnodet variant would also be able to download the zip.

@gzm55
Copy link
Contributor Author

gzm55 commented Oct 23, 2022

@ppalaga i add a post-install script bin/mvnd-persist-native for detecting and moving the native mage as the default entry

gzm55 and others added 5 commits January 10, 2023 15:22
1. rename native binary to 'mvnd-native-<os>-<arch>'
2. add environment switch MVND_ENTRY_FALLBACK, default 'true' enables
   the fallback logic, set to 'false' to force execute the native mvnd.
3. rename mvnd.sh to mvnd
@gzm55
Copy link
Contributor Author

gzm55 commented Jan 10, 2023

I'd vote for evaluating the performance impact of wrapping mvnd binary inside a shell script before merging this.

@ppalaga I did some basic profiling, comparing to directly execute native binaries:

  • MVND_ENTRY_FALLBACK=false: fallback script increases about 50ms on linux and macos
  • MVND_ENTRY_FALLBACK=true: fallback script increases about 60ms on macos
  • MVND_ENTRY_FALLBACK=true: fallback script increases about 120ms on linux

@gzm55 gzm55 requested a review from ppalaga January 10, 2023 17:23
@ppalaga
Copy link
Contributor

ppalaga commented Jan 10, 2023

I'd dare to vote against making mvnd to point at a shell script by default. It is adding indirection and performance penalties on major platforms for which we have proper native executables, thus worsening both the user experience for the majority of users and reliable testability of the default setup.

I'd be open to accepting the following alternatives that I think also satisfy the original original requirement of having a universal single entry mvnd:

  1. Enhance the existing shell scripts to fall back to native executables (if available and working on the current platform) without renaming them.
  2. Provide install scripts that replace the native mvnd with a shell script if the native mvnd does not work on or does not match the current platform.

What do others think?

@gzm55
Copy link
Contributor Author

gzm55 commented Jan 11, 2023

  1. Enhance the existing shell scripts to fall back to native executables (if available and working on the current platform) without renaming them.
  2. Provide install scripts that replace the native mvnd with a shell script if the native mvnd does not work on or does not match the current platform.

Hi @ppalaga , I would like to make this pr focus on the 1st point, implementing a proper fallback logic in the script entrypoint of the current file layout (mvnd.sh and mvnd.cmd). And create another issue/pr for the 2nd point to discuss and dev the post installing process.

@gzm55
Copy link
Contributor Author

gzm55 commented Jan 11, 2023

@ppalaga @gnodet

  1. Enhance the existing shell scripts to fall back to native executables (if available and working on the current platform) without renaming them.

done in this pr

  1. Provide install scripts that replace the native mvnd with a shell script if the native mvnd does not work on or does not match the current platform.

record in the issue #771

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Thanks looks great! I left some minor comment text suggestions inline.

dist/src/main/distro/bin/mvnd.cmd Outdated Show resolved Hide resolved
dist/src/main/distro/bin/mvnd.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Great work, thanks a lot, @gzm55!

@gnodet gnodet self-requested a review January 16, 2023 08:15
@@ -97,6 +97,7 @@
<include>mvnd</include>
<include>mvnd.exe</include>
</directory>
<file touch="platform-${os.detected.name}-${os.detected.arch}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flag file is used to detect whether the bin/mvnd is built for the current os and arch.

@gnodet gnodet merged commit 6f28a18 into apache:master Jan 24, 2023
gnodet pushed a commit that referenced this pull request Jan 24, 2023
* Add script mvnd-auto to auto select native or pure java mvnd

* Move fallback logic into main entry script

1. rename native binary to 'mvnd-native-<os>-<arch>'
2. add environment switch MVND_ENTRY_FALLBACK, default 'true' enables
   the fallback logic, set to 'false' to force execute the native mvnd.
3. rename mvnd.sh to mvnd

* change entry name on windows

* Add script mvnd-persist-native for moving the native image to the default entry path

* improve platform detect

* fix error on dash

* rollback default entry to the native image

* use MVND_CLIENT switch to control the selection of mvnd client

* improve comment docs as suggestion
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.

None yet

3 participants