8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 #18496

simonis · 2024-03-26T19:14:23Z

Since JDK-8251462: Simplify compilation policy, introduced in JDK 17, no native wrappers are generated any more if running with -XX:-TieredCompilation (i.e. native methods are not compiled any more).

The attached JMH benchmark demonstrate that native method calls became twice as expensive with JDK 17:

  public static native void emptyStaticNativeMethod();

  @Benchmark
  public static void baseline() {
  }

  @Benchmark
  public static void staticMethodCallingStatic() {
    emptyStaticMethod();
  }

  @Benchmark
  public static void staticMethodCallingStaticNative() {
    emptyStaticNativeMethod();
  }

  @Benchmark
  @Fork(jvmArgsAppend = "-XX:-TieredCompilation")
  public static void staticMethodCallingStaticNativeNoTiered() {
    emptyStaticNativeMethod();
  }

  @Benchmark
  @Fork(jvmArgsAppend = "-XX:+PreferInterpreterNativeStubs")
  public static void staticMethodCallingStaticNativeIntStub() {
    emptyStaticNativeMethod();
  }

JDK 11

Benchmark                                           Mode  Cnt   Score   Error  Units
NativeCall.baseline                                 avgt    5   0.390 ± 0.016  ns/op
NativeCall.staticMethodCallingStatic                avgt    5   1.693 ± 0.053  ns/op
NativeCall.staticMethodCallingStaticNative          avgt    5  10.287 ± 0.754  ns/op
NativeCall.staticMethodCallingStaticNativeNoTiered  avgt    5   9.966 ± 0.248  ns/op
NativeCall.staticMethodCallingStaticNativeIntStub   avgt    5  20.384 ± 0.444  ns/op

JDK 17 & 21

Benchmark                                           Mode  Cnt   Score   Error  Units
NativeCall.baseline                                 avgt    5   0.390 ± 0.017  ns/op
NativeCall.staticMethodCallingStatic                avgt    5   1.852 ± 0.272  ns/op
NativeCall.staticMethodCallingStaticNative          avgt    5  10.648 ± 0.661  ns/op
NativeCall.staticMethodCallingStaticNativeNoTiered  avgt    5  20.657 ± 1.084  ns/op
NativeCall.staticMethodCallingStaticNativeIntStub   avgt    5  22.429 ± 0.991  ns/op

The issue can bee seen if we run with -XX:+PrintCompilation -XX:+PrintInlining. With JDK 11 we get the following output for -XX:+TieredCompilation:

    172  111    b  3       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                              @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method
    172  112     n 0       io.simonis.NativeCall::emptyStaticNativeMethod (native)   (static)
    173  113    b  4       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                              @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method
    173  111       3       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)   made not entrant

As you can see, the native wrapper for NativeCall::emptyStaticNativeMethod() gets compiled with compiled id 112. If we run with -XX:-TieredCompilation:

    117    5    b        io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                            @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method
    117    6     n       io.simonis.NativeCall::emptyStaticNativeMethod (native)   (static)

There's still a native wrapper created with compile id 6.

With JDK 17 and later, the -XX:+PrintCompilation output looks similar for the default -XX:+TieredCompilation case:

     56   26    b  3       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                              @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method
     56   27     n 0       io.simonis.NativeCall::emptyStaticNativeMethod (native)   (static)
     56   28    b  4       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                              @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method
     56   26       3       io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)   made not entrant

But with -XX:-TieredCompilation, we don't generate the native wrapper any more:

     58    5    b        io.simonis.NativeCall::staticMethodCallingStaticNative (4 bytes)
                            @ 0   io.simonis.NativeCall::emptyStaticNativeMethod (0 bytes)   native method

Which basically means that we're always invoking the native method through the interpreter stub.

There are certainly different ways to fix this issue. The one proposed in this PR seemed simple and non-intrusive to me but I'm open for better alternatives. I'd especially appreciate if @veresov could take a look and advice.

Once we agree on the problem and a solution for it, I can also add a test and potentially the JMH benchmark from the JBS issue to this PR.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 (Bug - P3)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Igor Veresov (@veresov - Reviewer)
Vladimir Ivanov (@iwanowww - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18496/head:pull/18496
$ git checkout pull/18496

Update a local copy of the PR:
$ git checkout pull/18496
$ git pull https://git.openjdk.org/jdk.git pull/18496/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18496

View PR using the GUI difftool:
$ git pr show -t 18496

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18496.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-26T19:15:22Z

👋 Welcome back simonis! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-26T19:15:44Z

@simonis This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462

Reviewed-by: kvn, iveresov, vlivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 29 new commits pushed to the master branch:

bf93e77: 8329118: Run MessageFormat additional subformat pattern tests under en_US locale
991e04e: 8327383: Clean up _Stalled and _Spinner fields
4eefda9: 8327779: Remove deprecated internal field sun.security.x509.X509Key.key
341dd57: 8325883: Move Monitor Deflation reporting out of safepoint cleanup
7ac2f91: 8329210: Delete Redundant Printer Dialog Modality Test
85cb4a9: 8247449: Revisit the argument processing logic for MetaspaceShared::disable_optimized_module_handling()
aa595db: 8328507: Move StackWatermark code from safepoint cleanup
2af0312: 8328619: sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java failed with BindException: Address already in use
7c7b961: 8329191: JVMCI compiler warning is truncated
2b79c22: 8327505: Test com/sun/jmx/remote/NotificationMarshalVersions/TestSerializationMismatch.java fails
... and 19 more: https://git.openjdk.org/jdk/compare/89e0889ab3beb96103ee381d9a54614e49e6d9d7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-03-26T19:16:30Z

@simonis The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-26T19:19:42Z

Webrevs

…tion after JDK-8251462

openjdk · 2024-03-26T22:39:27Z

@simonis Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

src/hotspot/share/compiler/compilationPolicy.cpp

vnkozlov

We need a regression test to catch this in a future.

veresov · 2024-03-27T16:07:57Z

Have you found out why exact are the wrappers not created with -XX:-TieredCompilation ? I don't see any conditional with is_native() that would prevent that?

veresov · 2024-03-27T16:18:37Z

Regarding the solution. You seem to be tapping into the is_method_profiled() functionality and applying the threshold rule meant for methods with MDOs, which is a bit hacky. How about a more straightforward way:

diff --git a/src/hotspot/share/compiler/compilationPolicy.cpp b/src/hotspot/share/compiler/compilationPolicy.cpp
index d61de7cc866..57173ed621c 100644
--- a/src/hotspot/share/compiler/compilationPolicy.cpp
+++ b/src/hotspot/share/compiler/compilationPolicy.cpp
@@ -1026,7 +1026,7 @@ CompLevel CompilationPolicy::common(const methodHandle& method, CompLevel cur_le
   if (force_comp_at_level_simple(method)) {
     next_level = CompLevel_simple;
   } else {
-    if (is_trivial(method)) {
+    if (is_trivial(method) || method->is_native()) {
       next_level = CompilationModeFlag::disable_intermediate() ? CompLevel_full_optimization : CompLevel_simple;
     } else {
       switch(cur_level) {

What do you think? Would that work?

simonis · 2024-03-27T19:10:58Z

Regarding the solution. You seem to be tapping into the is_method_profiled() functionality and applying the threshold rule meant for methods with MDOs, which is a bit hacky. How about a more straightforward way:

diff --git a/src/hotspot/share/compiler/compilationPolicy.cpp b/src/hotspot/share/compiler/compilationPolicy.cpp
index d61de7cc866..57173ed621c 100644
--- a/src/hotspot/share/compiler/compilationPolicy.cpp
+++ b/src/hotspot/share/compiler/compilationPolicy.cpp
@@ -1026,7 +1026,7 @@ CompLevel CompilationPolicy::common(const methodHandle& method, CompLevel cur_le
   if (force_comp_at_level_simple(method)) {
     next_level = CompLevel_simple;
   } else {
-    if (is_trivial(method)) {
+    if (is_trivial(method) || method->is_native()) {
       next_level = CompilationModeFlag::disable_intermediate() ? CompLevel_full_optimization : CompLevel_simple;
     } else {
       switch(cur_level) {

What do you think? Would that work?

I think it will work, but wouldn't that instantly create a native wrapper for every native method? Shouldn't we only create native wrappers for hot native methods?

simonis · 2024-03-27T19:13:11Z

Have you found out why exact are the wrappers not created with -XX:-TieredCompilation ? I don't see any conditional with is_native() that would prevent that?

The reason why they are not created is because native methods have no MDO so we always bail out here:

      case CompLevel_full_profile:
        {
          MethodData* mdo = method->method_data();
          if (mdo != nullptr) {
            if (mdo->would_profile() || CompilationModeFlag::disable_intermediate()) {
              int mdo_i = mdo->invocation_count_delta();
              int mdo_b = mdo->backedge_count_delta();
              if (Predicate::apply(method, cur_level, mdo_i, mdo_b)) {
                next_level = CompLevel_full_optimization;
              }
            } else {
              next_level = CompLevel_full_optimization;
            }
          }

veresov · 2024-03-27T19:48:37Z

I think it will work, but wouldn't that instantly create a native wrapper for every native method? Shouldn't we only create native wrappers for hot native methods?

To get here it will have to be already relatively hot. And since we don't need to profile it I think it would be fair to treat it exactly the same way we treat trivial methods.

simonis · 2024-03-28T12:05:20Z

I think it will work, but wouldn't that instantly create a native wrapper for every native method? Shouldn't we only create native wrappers for hot native methods?

To get here it will have to be already relatively hot. And since we don't need to profile it I think it would be fair to treat it exactly the same way we treat trivial methods.

OK, then better early than never :)

veresov

LGTM

vnkozlov · 2024-03-28T19:13:28Z

I submitted our testing for v03 version.

vnkozlov

My tier1-3,xcomp,stress testing passed.

iwanowww

Good catch! Looks good.

simonis · 2024-03-30T12:46:48Z

/integrate

openjdk · 2024-03-30T12:47:52Z

Going to push as commit f2e5808.
Since your change was applied there have been 38 commits pushed to the master branch:

37c2279: 8329150: Remove CDS support for LatestMethodCache
20cb6e7: 8329337: Problem list BufferStrategyExceptionTest.java on Windows
d1b51e3: 8321550: Update several runtime/cds tests to use vm flags or mark as flagless
8a0ef81: 8326627: Document Double/Float.valueOf(String) behavior for numeric strings with non-ASCII digits
5b05f8e: 8329112: Clean up CDS checking of unsupported module options
59c2aff: 8323624: ProviderList.ServiceList does not need to be a list
418deaf: 8328361: Use memset() in method CardTable::dirty_MemRegion()
245514d: 8328953: JEditorPane.read throws ChangedCharSetException
df01cc5: 8323576: [Windows] Fallthrough to ::abort instead of os::infinite_sleep for noreturn methods
bf93e77: 8329118: Run MessageFormat additional subformat pattern tests under en_US locale
... and 28 more: https://git.openjdk.org/jdk/compare/89e0889ab3beb96103ee381d9a54614e49e6d9d7...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-30T12:47:58Z

@simonis Pushed as commit f2e5808.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Mar 26, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 26, 2024

8329126: No native wrappers generated anymore with -XX:-TieredCompila…

157e124

…tion after JDK-8251462

simonis force-pushed the JDK-8251462-fix-native-wrapper-for-nontiered branch from 7eb0d11 to 157e124 Compare March 26, 2024 22:39

simonis changed the title ~~8251462: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462~~ 8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 Mar 26, 2024

TobiHartmann reviewed Mar 27, 2024

View reviewed changes

src/hotspot/share/compiler/compilationPolicy.cpp Outdated Show resolved Hide resolved

Fix indentation

5b017d5

vnkozlov reviewed Mar 27, 2024

View reviewed changes

Compile native methods like trivial methods and added JTreg test

3324731

veresov approved these changes Mar 28, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 28, 2024

vnkozlov approved these changes Mar 28, 2024

View reviewed changes

iwanowww approved these changes Mar 29, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 30, 2024

openjdk bot closed this Mar 30, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Mar 30, 2024

openjdk bot removed the rfr Pull request is ready for review label Mar 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 #18496

8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 #18496

simonis commented Mar 26, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 26, 2024

openjdk bot commented Mar 26, 2024 •

edited

Loading

openjdk bot commented Mar 26, 2024

mlbridge bot commented Mar 26, 2024 •

edited

Loading

openjdk bot commented Mar 26, 2024

vnkozlov left a comment

veresov commented Mar 27, 2024

veresov commented Mar 27, 2024

simonis commented Mar 27, 2024

simonis commented Mar 27, 2024

veresov commented Mar 27, 2024

simonis commented Mar 28, 2024

veresov left a comment

vnkozlov commented Mar 28, 2024

vnkozlov left a comment

iwanowww left a comment

simonis commented Mar 30, 2024

openjdk bot commented Mar 30, 2024

openjdk bot commented Mar 30, 2024

8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 #18496

8329126: No native wrappers generated anymore with -XX:-TieredCompilation after JDK-8251462 #18496

Conversation

simonis commented Mar 26, 2024 • edited by openjdk bot Loading

JDK 11

JDK 17 & 21

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 26, 2024

openjdk bot commented Mar 26, 2024 • edited Loading

openjdk bot commented Mar 26, 2024

mlbridge bot commented Mar 26, 2024 • edited Loading

Webrevs

openjdk bot commented Mar 26, 2024

vnkozlov left a comment

Choose a reason for hiding this comment

veresov commented Mar 27, 2024

veresov commented Mar 27, 2024

simonis commented Mar 27, 2024

simonis commented Mar 27, 2024

veresov commented Mar 27, 2024

simonis commented Mar 28, 2024

veresov left a comment

Choose a reason for hiding this comment

vnkozlov commented Mar 28, 2024

vnkozlov left a comment

Choose a reason for hiding this comment

iwanowww left a comment

Choose a reason for hiding this comment

simonis commented Mar 30, 2024

openjdk bot commented Mar 30, 2024

openjdk bot commented Mar 30, 2024

simonis commented Mar 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 26, 2024 •

edited

Loading

mlbridge bot commented Mar 26, 2024 •

edited

Loading