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

8255959: Timeouts in VectorConversion tests #1079

Conversation

TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Nov 5, 2020

We observed many timeouts in the following test/jdk/jdk/incubator/vector tests:
Vector128ConversionTests.java
Vector256ConversionTests.java
Vector512ConversionTests.java
Vector64ConversionTests.java
VectorMaxConversionTests.java
Some machines don't support vector instructions or fewer of them and C2 uses slower alternatives.

Maybe there are options to make the tests faster, but I just propose to use a larger timeout value for now.


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 5, 2020

👋 Welcome back mdoerr! 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 Nov 5, 2020
@openjdk
Copy link

openjdk bot commented Nov 5, 2020

@TheRealMDoerr 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

@TheRealMDoerr
Copy link
Contributor Author

/label hotspot-compiler

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

openjdk bot commented Nov 5, 2020

@TheRealMDoerr
The hotspot-compiler label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Nov 5, 2020

Webrevs

@PaulSandoz
Copy link
Member

I have not observed such timeouts in our test infrastructure, but was wondering if this may cause such issues. Perhaps PPC does not support the conversion intrinsics?

The timeout value is quite high, 1800 (i cannot recall what the default is). Ideally we should split this test per species. I regret that this test is not produced from a template.

Would you mind first if we can try a quick experiment to reduce the time taken? In AbstractVectorConversionTest there is a loop using an upper bound of INVOC_COUNT, perhaps if any of the species input to the relevant methods have a length > the length of the corresponding preferred species, then we can reduce the loop count.

@TheRealMDoerr
Copy link
Contributor Author

At the moment, neither PPC nor s390 support any conversion intrinsics. Modern s390 (or
z/Architecture to be more precise) machines have vector instructions, but nobody implemented them in hotspot. So s390 uses the regular 8 Byte registers for vectors. PPC only uses 16 Byte vectors on modern hardware (8 Byte on older hardware).

Default timeout is 1200 and I found out that 50% more makes the tests happy on all our machines.

We could do an experiment, but I'm not familiar with the test.

@PaulSandoz
Copy link
Member

Perhaps the following patch might help.

Still for say 512 conversion test on my mac that has no AVX512 support the test runs (including compilation) in about 60s. With the patch it reduces to about 40s.

If you run jtreg in verbose mode, -va it should output individual test times. Perhaps some are taking longer than others?

diff --git a/test/jdk/jdk/incubator/vector/AbstractVectorConversionTest.java b/test/jdk/jdk/incubator/vector/AbstractVectorConversionTest.java
index d1303bfd295..1754af2110a 100644
--- a/test/jdk/jdk/incubator/vector/AbstractVectorConversionTest.java
+++ b/test/jdk/jdk/incubator/vector/AbstractVectorConversionTest.java
@@ -551,7 +551,8 @@ abstract class AbstractVectorConversionTest {
       int m = Math.max(dst_species_len,src_species_len) / Math.min(src_species_len,dst_species_len);
 
       int [] parts = getPartsArray(m, is_contracting_conv);
-      for (int ic = 0; ic < INVOC_COUNT; ic++) {
+      int count = invocationCount(INVOC_COUNT, SPECIES, OSPECIES);
+      for (int ic = 0; ic < count; ic++) {
          for (int i=0, j=0; i < in_len; i += src_species_len, j+= dst_species_len) {
             int part = parts[i % parts.length];
             var av = Vector64ConversionTests.<I>vectorFactory(unboxed_a, i, SPECIES);
@@ -592,7 +593,8 @@ abstract class AbstractVectorConversionTest {
       int m = Math.max(dst_vector_size,src_vector_size) / Math.min(dst_vector_size, src_vector_size);
 
       int [] parts = getPartsArray(m, is_contracting_conv);
-      for (int ic = 0; ic < INVOC_COUNT; ic++) {
+      int count = invocationCount(INVOC_COUNT, SPECIES, OSPECIES);
+      for (int ic = 0; ic < count; ic++) {
         for (int i = 0, j=0; i < in_len; i += src_vector_lane_cnt, j+= dst_vector_lane_cnt) {
           int part = parts[i % parts.length];
           var av = Vector64ConversionTests.<I>vectorFactory(unboxed_a, i, SPECIES);
@@ -609,4 +611,15 @@ abstract class AbstractVectorConversionTest {
       }
       assertResultsEquals(boxed_res, boxed_ref, dst_vector_lane_cnt);
     }
+
+    static int invocationCount(int c, VectorSpecies<?>... species) {
+        return Arrays.asList(species).stream().allMatch(AbstractVectorConversionTest::leqPreferred)
+                ? c
+                : Math.min(c, c / 100);
+    }
+
+    static boolean leqPreferred(VectorSpecies<?> species) {
+        VectorSpecies<?> preferred = VectorSpecies.ofPreferred(species.elementType());
+        return species.length() <= preferred.length();
+    }
 }

@TheRealMDoerr
Copy link
Contributor Author

Hi Paul,
your proposal helps a bit. Test has passed on some machines, but not on all ones.
"contracting_conversion_scalar" is still very prominent on PPC. Do we need to implement intrinsics to get this fast?

In addition, there's another timeout in AddTest.java on x86:
https://bugs.openjdk.java.net/browse/JDK-8255915

So it seems like there's more work to do.
Suggestions?

@TheRealMDoerr
Copy link
Contributor Author

Situation has improved with 8256581: Refactor vector conversion tests.
I'm closing this PR. We may need a new one to increase some timeout values for specific platforms.

@TheRealMDoerr TheRealMDoerr deleted the 8255959_VectorConversionTimeout branch November 20, 2020 09:49
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 rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants