Skip to content
This repository has been archived by the owner. It is now read-only.

8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled #139

Closed
wants to merge 1 commit into from

Conversation

@casparcwang
Copy link
Contributor

@casparcwang casparcwang commented Jan 30, 2021

https://bugs.openjdk.java.net/browse/JDK-8260473

Function "PhaseVector::expand_vunbox_node" creates a LoadNode, but forgets to make the LoadNode to pass gc barriers.

Testing: all Vector API related tests have passed.

Original pr: openjdk/jdk#2253


Progress

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

Issue

  • JDK-8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled

Reviewers

Contributors

  • Stuart Monteith <smonteith@openjdk.org>
  • Wang Chao <casparcwang@tencent.com>

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/139/head:pull/139
$ git checkout pull/139

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 30, 2021

👋 Welcome back casparcwang! 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 label Jan 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

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

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 30, 2021

/cc hotspot-gc

/contributor add Wang Chao casparcwang@tencent.com
/contributor add @stooart-mon

@openjdk openjdk bot added the hotspot-gc label Jan 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

@casparcwang
The hotspot-gc label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

@casparcwang Could not parse Wang Chao casparcwang@tencent.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

@casparcwang
Contributor Stuart Monteith <smonteith@openjdk.org> successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 30, 2021

Webrevs

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 30, 2021

/contributor add @casparcwang

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

@casparcwang Could not parse @casparcwang as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@casparcwang casparcwang changed the title 8260473: [vector] ZGC: VectorReshape test produces incorrect results … 8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled Jan 30, 2021
@DamonFool
Copy link
Member

@DamonFool DamonFool commented Jan 30, 2021

/contributor add Wang Chao casparcwang@tencent.com

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

@DamonFool Only the author (@casparcwang) is allowed to issue the contributor command.

Copy link

@iwanowww iwanowww left a comment

Overall, looks good.

* @modules jdk.incubator.vector
* @modules java.base/jdk.internal.vm.annotation
* @run testng/othervm -XX:CompileCommand=compileonly,jdk/incubator/vector/ByteVector.fromByteBuffer
* -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+UseZGC -Xbatch -Xmx256m VectorRebracket128Test

Why -XX:CICompilerCount=1 and -Xmx256m are needed?

Copy link
Member

@DamonFool DamonFool Jan 31, 2021

Why -XX:CICompilerCount=1 and -Xmx256m are needed?

Thanks @iwanowww for your review.

I discussed the same question with @casparcwang offline.
The reason is:

  • Small heap (-Xmx256m) will help to trigger a gc.
  • compileonly and compilercount=1 will let the VM run slow enough to wait for a gc to be finished.

And @casparcwang told me that this bug seems not to be reproduced every time without these JVM args.
Thanks.

Copy link

@iwanowww iwanowww Feb 1, 2021

compileonly and compilercount=1 will let the VM run slow enough to wait for a gc to be finished.

That's a strange way to provoke the bug. You could just increase the number of iterations instead.

But the right way to fix it is to stress ZGC to continuously run in the background while the test case aggressively unboxes vectors in compiled code. -Xmx256m helps with that while -XX:CICompilerCount=1 is irrelevant.

Copy link
Contributor Author

@casparcwang casparcwang Feb 1, 2021

compileonly and compilercount=1 will let the VM run slow enough to wait for a gc to be finished.

That's a strange way to provoke the bug. You could just increase the number of iterations instead.

But the right way to fix it is to stress ZGC to continuously run in the background while the test case aggressively unboxes vectors in compiled code. -Xmx256m helps with that while -XX:CICompilerCount=1 is irrelevant.

Yes, it's very weird to provoke the bug like this. If CICompilerCount=1 is removed, the test failed 60% roughly on my machine.
And the iteration has already changed from 100 to 1000, the run time of the test is nearly 30s on release version of jvm.

If I add the following patch, the test always fails on my machine,

diff --git a/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java b/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
index 1843ec0..959b29a 100644
--- a/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
+++ b/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
@@ -44,7 +44,7 @@ import jdk.internal.vm.annotation.ForceInline;
  * @modules jdk.incubator.vector
  * @modules java.base/jdk.internal.vm.annotation
  * @run testng/othervm -XX:CompileCommand=compileonly,jdk/incubator/vector/ByteVector.fromByteBuffer
- *      -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+UseZGC -Xbatch -Xmx256m VectorRebracket128Test
+ *      -XX:-TieredCompilation -XX:+UseZGC -Xmx256m VectorRebracket128Test
  */
 
 @Test
@@ -125,6 +125,14 @@ public class VectorRebracket128Test {
     @ForceInline
     static <E,F>
     void testVectorRebracket(VectorSpecies<E> a, VectorSpecies<F> b, byte[] input, byte[] output) {
+        new Thread(() -> {
+            while (true) {
+                try {
+                    System.gc();
+                    Thread.sleep(100);
+                } catch (Exception e) {}
+            }
+        }).start();
         Vector<E> av = a.fromByteArray(input, 0, ByteOrder.nativeOrder());
         int block;
         assert(input.length == output.length);

Copy link
Contributor Author

@casparcwang casparcwang Feb 1, 2021

sorry for the wrong patch above, the failed reason of the patch above is due to stack creation failure (create 1000 threads). The following is the right stress gc patch.

diff --git a/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java b/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
index 6b266db..a761ea2 100644
--- a/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
+++ b/test/hotspot/jtreg/compiler/vectorapi/VectorRebracket128Test.java
@@ -44,7 +44,7 @@ import jdk.internal.vm.annotation.ForceInline;
  * @modules jdk.incubator.vector
  * @modules java.base/jdk.internal.vm.annotation
  * @run testng/othervm -XX:CompileCommand=compileonly,jdk/incubator/vector/ByteVector.fromByteBuffer
- *      -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+UseZGC -Xbatch -Xmx256m VectorRebracket128Test
+ *      -XX:-TieredCompilation -XX:+UseZGC -Xmx256m VectorRebracket128Test
  */
 
 @Test
@@ -59,6 +59,19 @@ public class VectorRebracket128Test {
     static final VectorSpecies<Byte> bspec128 = ByteVector.SPECIES_128;
     static final VectorSpecies<Short> sspec128 = ShortVector.SPECIES_128;
 
+    static {
+        Thread t = new Thread(() -> {
+            while (true) {
+                try {
+                    System.gc();
+                    Thread.sleep(100);
+                } catch (Exception e) {}
+            }
+        });
+        t.setDaemon(true);
+        t.start();
+    }
+
     static <T> IntFunction<T> withToString(String s, IntFunction<T> f) {
         return new IntFunction<T>() {
             @Override

Copy link

@iwanowww iwanowww Feb 1, 2021

Good. Please, file a follow-up RFE to improve the test.

Copy link
Member

@DamonFool DamonFool Feb 2, 2021

Good. Please, file a follow-up RFE to improve the test.

OK.
I will help to file a JBS bug once the fix has been merged into the jdk mainline.
It will be only fixed in the jdk17, right?
Thanks.

Copy link
Member

@DamonFool DamonFool Feb 4, 2021

Good. Please, file a follow-up RFE to improve the test.

The RFE has been filed here: https://bugs.openjdk.java.net/browse/JDK-8261152
Thanks.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

⚠️ @casparcwang the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8260473
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

@openjdk openjdk bot commented Jan 30, 2021

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

8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled

Co-authored-by: Stuart Monteith <smonteith@openjdk.org>
Co-authored-by: Wang Chao <casparcwang@tencent.com>
Reviewed-by: vlivanov, neliasso

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@iwanowww, @neliasso) 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 label Jan 30, 2021
@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 31, 2021

/contributor add Wang Chao casparcwang@tencent.com

@openjdk
Copy link

@openjdk openjdk bot commented Jan 31, 2021

@casparcwang Could not parse Wang Chao casparcwang@tencent.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

Copy link
Contributor

@neliasso neliasso left a comment

Approved.

Now awaiting release team approval.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 31, 2021

@neliasso Only the author (@casparcwang) is allowed to issue the contributor command.

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Feb 1, 2021

/contributor add Wang Chao casparcwang@tencent.com

@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@casparcwang
Contributor Wang Chao <casparcwang@tencent.com> successfully added.

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Feb 1, 2021

/integrate

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Feb 1, 2021

/sponsor

@openjdk openjdk bot added the sponsor label Feb 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@casparcwang
Your change (at version 7bc0a87) is now ready to be sponsored by a Committer.

@openjdk openjdk bot closed this Feb 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 1, 2021

@DamonFool @casparcwang Pushed as commit 0fdf9cd.

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

@iwanowww
Copy link

@iwanowww iwanowww commented Feb 2, 2021

It will be only fixed in the jdk17, right?

Yes, I'm OK with that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants