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

8257147: [TESTBUG] Set a larger default loop count for the VectorAPI jtreg tests #1621

Closed
wants to merge 2 commits into from

Conversation

XiaohongGong
Copy link

@XiaohongGong XiaohongGong commented Dec 4, 2020

The current default loop count of VectorAPI tests is too small (10 for load/store tests, 100 for arithmetic tests) to make the tests compiled with C2. This makes some potential issues not be reported as expected.

This patch fixes it by setting a larger default iteration.

The whole tests running time increases with this patch. Here is the results on two different types of machines:

Running with "-conc:10":

             Before     After
System A     5m30s      6m43s
System B     5m1s       6m31s

Running with "-conc:1":

             Before     After
System A     26m52s     45m6s
System B     30m3s      43m30s

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8257147: [TESTBUG] Set a larger default loop count for the VectorAPI jtreg tests

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1621/head:pull/1621
$ git checkout pull/1621

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 4, 2020

👋 Welcome back xgong! 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 openjdk bot added the rfr Pull request is ready for review label Dec 4, 2020
@openjdk
Copy link

openjdk bot commented Dec 4, 2020

@XiaohongGong To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

@XiaohongGong
Copy link
Author

/label add hotspot-compiler

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 8, 2020
@openjdk
Copy link

openjdk bot commented Dec 8, 2020

@XiaohongGong
The hotspot-compiler label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Dec 8, 2020

Webrevs

@PaulSandoz
Copy link
Member

It's good to revisit this, but I am concerned about the increase in test execution time. I wonder if we can approach this a little differently.

First, I think we should switch off tiered compilation, -XX:-TieredCompilation (maybe reconsider later some limited form of execution with tiered compilation enabled).
For such a configuration I observe, when using the current invocation count and looking at inline traces, far more lines with intrinsic VectorSupport methods.

Second, we could lower the compile threshold from 10000 e.g. -XX:CompileThreshold=1000. We can grep the inline trace and count the lines containing intrinsic VectorSupport methods.

Using both these approaches i think we can increase intrinsification without such a large increase in test execution time.

WDYT?

@XiaohongGong
Copy link
Author

It's good to revisit this, but I am concerned about the increase in test execution time. I wonder if we can approach this a little differently.

First, I think we should switch off tiered compilation, -XX:-TieredCompilation (maybe reconsider later some limited form of execution with tiered compilation enabled).
For such a configuration I observe, when using the current invocation count and looking at inline traces, far more lines with intrinsic VectorSupport methods.

Second, we could lower the compile threshold from 10000 e.g. -XX:CompileThreshold=1000. We can grep the inline trace and count the lines containing intrinsic VectorSupport methods.

Using both these approaches i think we can increase intrinsification without such a large increase in test execution time.

WDYT?

Hi @PaulSandoz , thanks for looking at this issue! It's ok for me to try these two options. I will have a try and update the results later. Thanks!

@XiaohongGong
Copy link
Author

Hi @PaulSandoz , I tested the jtreg tests with these two options these days, and the results do not show better as expected.

With -XX:-TieredCompilation, the tests can work well without increasing the loop count. However, I think it's not an optimal solution. First, with this flag, some issues that might only happen with tiered compilation might not be reported as expected (Currently we do have met a similar issue). Second, a better way to add the option for the test is adding it to the test file (e.g. @run testng/othervm -XX:-TieredCompilation). However, this might override the same options people specified when running jtreg.

For -XX:CompileThreshold=1000, unfortunately it seems this flag doesn't have any effect that I still need to increase the loop count to make the tests effective. Good news is that I found the flags -XX:CompileThresholdScaling=0.1 -Xbatch can work well even if I do not increase the loop count. However, this still increase the whole execution time a lot when running with -conc:1. Here is part of the comparison for the time:

Running with "-conc:10":

             Before     After
System A     5m21s      5m29s
System B     5m16s      6m7s

Running with "-conc:1":

             Before     After
System A     33m20s     62m14s
System B     36m17s     71m59s

So do you have tried these before? Or any better idea about this? Thanks!

Xiaohong Gong

@PaulSandoz
Copy link
Member

Thanks for looking at this in more detail. Before suggesting i did try some rough experiments which did indicate some upside to running without tiered with reduced compiler thresholds, but i only selected a few test classes to run against.

It's true that some issues have been found in tiered compilation. However, i think there is a tradeoff, as these tests are really designed exercise C2.

My suggestion for now is to just turn off tiered compilation, which gets us reasonably far, and perhaps keep pushing on compiler threshold investigation as a follow up.

I will follow up with our Hotspot test engineers to see if there is a way to run these tests less frequently under tiered compilation (as we do for other flags to stress the HotSpot compilers, such as -Xcomp).

nsjian pushed a commit to nsjian/jdk16 that referenced this pull request Dec 18, 2020
After applying [1], some Vector API tests fail with SIGILL on SVE
system. The SIGILL was triggered by verify_ptrue before c2 compiled
function returns, which means that the preserved p7 register (as ptrue)
has been clobbered before returning to c2 compiled code. (p7 is not
preserved cross function calls, and system calls [2]).

Currently we try to reinitialize ptrue at each entrypoint of returning
from non-c2 compiled code, which indicating possible C or system calls.
However, there's still one entrypoint missing, exception handling, as
we may jump to c2 compiled code for exception handler. See
OptoRuntime::generate_exception_blob().

Adding reinitialize_ptrue before jumping back to c2 compiled code in
generate_exception_blob() could solve those Vector API failures.
Actually I had that in my initial test patch [3], I don't know why I
missed that in final patch... I reran tests with the same approach of
[3] and found that there's still something missing, the
nmethod_entry_barrier() in c2 function prolog. The barrier may call to
runtime code (see generate_method_entry_barrier()). To reduce the risk
of missing such reinitialize_ptrue in newly added code in future, I
think it would be better to do the reinitialize in
pop_call_clobbered_registers().

P.S. the SIGILL message is also not clear, it should print detailed
message as indicated by MacroAssembler::stop() call. This is caused by
JDK-8255711 removing the message printing code. This patch also adds it
back, so that it could print detailed message for abort.

Tested with tier1-3 on SVE hardware. Also verified with the same
approach of patch [3] with jtreg tests hotspot_all_no_apps and
jdk:tier1-3 passed without incorrect ptrue value assertion failure.

[1] openjdk/jdk#1621
[2] https://github.com/torvalds/linux/blob/master/Documentation/arm64/sve.rst
[3] http://cr.openjdk.java.net/~njian/8231441/0001-RFC-Block-one-caller-save-register-for-C2.patch
@XiaohongGong
Copy link
Author

My suggestion for now is to just turn off tiered compilation, which gets us reasonably far, and perhaps keep pushing on compiler threshold investigation as a follow up.

OK, so I will update the patch with adding the "-XX:-TieredCompilation" for each test. As for the compiler threshold, I didn't have a deep investigation, but just looked at the compiler log with a simple case with it. It seems there are more uncommon-traps that might make deoptimization happen which might influences the performance a lot. Anyway, I'm not sure whether this is the main cause.

I will follow up with our Hotspot test engineers to see if there is a way to run these tests less frequently under tiered compilation (as we do for other flags to stress the HotSpot compilers, such as -Xcomp).

That's great, thanks for the support!

@XiaohongGong
Copy link
Author

Hi @PaulSandoz, I'v updated the patch. Could you please take a look at it again? Thanks so much!

Copy link
Member

@PaulSandoz PaulSandoz 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.

This got me wondering why -Xbatch was previously added. Perhaps we can revisit that afterwards with further investigation?

@openjdk
Copy link

openjdk bot commented Dec 23, 2020

@XiaohongGong 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:

8257147: [TESTBUG] Set a larger default loop count for the VectorAPI jtreg tests

Reviewed-by: psandoz

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 327 new commits pushed to the master branch:

  • 127582f: 8258913: ProblemList javax/swing/JComboBox/6559152/bug6559152.java on Linux-X64
  • cdb487a: 8258856: VM build without C1/C2 fails after JDK-8243205
  • 78c9fb9: 8258851: Mismatch in SunPKCS11 provider registration properties and actual implementation
  • fda0943: 8258839: Remove JVM option ExitVMOnVerifyError
  • cd94606: 8258186: Replace use of JNI_COMMIT mode with mode 0
  • e46edb5: 8258911: ProblemList serviceability/attach/RemovingUnixDomainSocketTest.java on Linux-X64
  • 91244cc: 8258557: Deproblemlist fixed problemlisted test
  • 2445735: 8258837: Remove JVM option DisableStartThread
  • a4e082e: 8253368: TLS connection always receives close_notify exception
  • 4ea8851: 8257928: Test image build failure with clang-10 due to -Wmisleading-indentation
  • ... and 317 more: https://git.openjdk.java.net/jdk/compare/d80ae05f617b35bd327e03869284de0c41adb94d...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@PaulSandoz) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 23, 2020
@XiaohongGong
Copy link
Author

Looks good.

Thanks for looking at it again!

This got me wondering why -Xbatch was previously added. Perhaps we can revisit that afterwards with further investigation?

Yeah, agree that a further investigation is needed. Thanks!

@XiaohongGong
Copy link
Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 24, 2020
@openjdk
Copy link

openjdk bot commented Dec 24, 2020

@XiaohongGong
Your change (at version 8bb9bac) is now ready to be sponsored by a Committer.

@nsjian
Copy link

nsjian commented Dec 28, 2020

/sponsor

@openjdk openjdk bot closed this Dec 28, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 28, 2020
@openjdk
Copy link

openjdk bot commented Dec 28, 2020

@nsjian @XiaohongGong Since your change was applied there have been 334 commits pushed to the master branch:

  • 97c99b5: 8216400: improve handling of IOExceptions in JavaCompiler.close()
  • b575dd8: 8258914: javax/net/ssl/DTLS/RespondToRetransmit.java timed out
  • 57217b5: Merge
  • c398a82: 8258916: javac/doclint reports broken HTML on multiline mailto links
  • 23b83c5: 8253954: javac crash when compiling code with enhanced switch expressions with option -Xjcov
  • 8b37c2c: 8257468: runtime/whitebox/TestWBDeflateIdleMonitors.java fails with Monitor should be deflated.: expected true to equal false
  • 9cd8e38: 8257521: runtime/logging/MonitorInflationTest.java crashed in MonitorList::unlink_deflated
  • 127582f: 8258913: ProblemList javax/swing/JComboBox/6559152/bug6559152.java on Linux-X64
  • cdb487a: 8258856: VM build without C1/C2 fails after JDK-8243205
  • 78c9fb9: 8258851: Mismatch in SunPKCS11 provider registration properties and actual implementation
  • ... and 324 more: https://git.openjdk.java.net/jdk/compare/d80ae05f617b35bd327e03869284de0c41adb94d...master

Your commit was automatically rebased without conflicts.

Pushed as commit 779ee11.

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

@XiaohongGong XiaohongGong deleted the JDK-8257147 branch December 28, 2020 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
3 participants