RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947
Hi Please help review the change for JDK-8193479 issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now. thanks, Sherman
It would add more confusion to a something that's already difficult to understand. It's OK as a temporary measure, but I don't think we want product code just to enable tests in hotspot. Can the hotspot folks modify their test, perhaps making this code part of the test? On Wed, Dec 13, 2017 at 4:01 PM, Xueming Shen <xueming.shen@oracle.com> wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now.
thanks, Sherman
David, Martin, webrev has been updated to fix the test directly. http://cr.openjdk.java.net/~sherman/8193479/webrev Assume I would need hotspot guys' help to review and push into hs repo? thanks, Sherman On 12/13/17, 4:52 PM, Martin Buchholz wrote:
It would add more confusion to a something that's already difficult to understand. It's OK as a temporary measure, but I don't think we want product code just to enable tests in hotspot. Can the hotspot folks modify their test, perhaps making this code part of the test?
On Wed, Dec 13, 2017 at 4:01 PM, Xueming Shen <xueming.shen@oracle.com <mailto:xueming.shen@oracle.com>> wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 <https://bugs.openjdk.java.net/browse/JDK-8193479> webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev <http://cr.openjdk.java.net/%7Esherman/8193479/webrev>
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now.
thanks, Sherman
Hi Sherman, On 14/12/2017 12:05 PM, Xueming Shen wrote:
David, Martin,
webrev has been updated to fix the test directly.
That seems to me to be invalidating the whole point of the test, which was a regression test for 6896617 to check that the optimizations put in place actually get applied. ?? But I'll leave that up to the compiler guys to decide.
Assume I would need hotspot guys' help to review and push into hs repo?
You'll need to fix in both jdk/hs and jdk/jdk as the breakage is now in both. Fix one and import to the other. But I still suggest adding the test to ProblemList.txt now so that this isn't broken in JDK 10 fork, then fix the actual test at a more leisurely pace in jdk/hs. But again I'll defer to compiler folk. David
thanks, Sherman
On 12/13/17, 4:52 PM, Martin Buchholz wrote:
It would add more confusion to a something that's already difficult to understand. It's OK as a temporary measure, but I don't think we want product code just to enable tests in hotspot. Can the hotspot folks modify their test, perhaps making this code part of the test?
On Wed, Dec 13, 2017 at 4:01 PM, Xueming Shen <xueming.shen@oracle.com <mailto:xueming.shen@oracle.com>> wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 <https://bugs.openjdk.java.net/browse/JDK-8193479> webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev <http://cr.openjdk.java.net/%7Esherman/8193479/webrev>
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now.
thanks, Sherman
On 12/13/17 6:52 PM, David Holmes wrote:
Hi Sherman,
On 14/12/2017 12:05 PM, Xueming Shen wrote:
David, Martin,
webrev has been updated to fix the test directly.
That seems to me to be invalidating the whole point of the test, which was a regression test for 6896617 to check that the optimizations put in place actually get applied. ??
But I'll leave that up to the compiler guys to decide.
Assume I would need hotspot guys' help to review and push into hs repo?
You'll need to fix in both jdk/hs and jdk/jdk as the breakage is now in both. Fix one and import to the other.
But I still suggest adding the test to ProblemList.txt now so that this isn't broken in JDK 10 fork, then fix the actual test at a more leisurely pace in jdk/hs.
I agree with David. Lets exclude it for now and fix it later properly without rush. Thanks Vladimir
But again I'll defer to compiler folk.
David
thanks, Sherman
On 12/13/17, 4:52 PM, Martin Buchholz wrote:
It would add more confusion to a something that's already difficult to understand. It's OK as a temporary measure, but I don't think we want product code just to enable tests in hotspot. Can the hotspot folks modify their test, perhaps making this code part of the test?
On Wed, Dec 13, 2017 at 4:01 PM, Xueming Shen <xueming.shen@oracle.com <mailto:xueming.shen@oracle.com>> wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 <https://bugs.openjdk.java.net/browse/JDK-8193479> webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev <http://cr.openjdk.java.net/%7Esherman/8193479/webrev>
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now.
thanks, Sherman
On 12/14/17, 9:58 AM, Vladimir Kozlov wrote:
On 12/13/17 6:52 PM, David Holmes wrote:
Hi Sherman,
On 14/12/2017 12:05 PM, Xueming Shen wrote:
David, Martin,
webrev has been updated to fix the test directly.
That seems to me to be invalidating the whole point of the test, which was a regression test for 6896617 to check that the optimizations put in place actually get applied. ??
But I'll leave that up to the compiler guys to decide.
Assume I would need hotspot guys' help to review and push into hs repo?
You'll need to fix in both jdk/hs and jdk/jdk as the breakage is now in both. Fix one and import to the other.
But I still suggest adding the test to ProblemList.txt now so that this isn't broken in JDK 10 fork, then fix the actual test at a more leisurely pace in jdk/hs.
I agree with David. Lets exclude it for now and fix it later properly without rush.
Thanks Vladimir
Thanks Vladimir, I have a webrev out there. But I will probably have to wait for the new jdk10 repo open next week. Or maybe the hs repo is still open for something like this? and you guys can help get it in directly there? Sherman -------------------------------------------- It appears the consensus is to put this one into the ProblemList and let the hotspot folks to fix the test case. issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev The corresponding hotspot/compiler issue filed: JDK-8193549. Thanks, Sherman ----------------------------------------------
Hi Sherman, On 14.12.2017 21:46, Xueming Shen wrote:
I have a webrev out there. But I will probably have to wait for the new jdk10 repo open next week.
Or maybe the hs repo is still open for something like this? and you guys can help get it in directly there?
I would say it's best to wait for the jdk10 repo to open.
It appears the consensus is to put this one into the ProblemList and let the hotspot folks to fix the test case.
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev
The corresponding hotspot/compiler issue filed: JDK-8193549.
The correct way is to create a subtask to quarantine the test. I've created one for you and closed (JDK-8193549) as duplicate: https://bugs.openjdk.java.net/browse/JDK-8193608 Please un-assign 8193479 if you don't plan to fix the test. Thanks, Tobias
Hi Sherman, On 15.12.2017 10:48, Tobias Hartmann wrote:
I would say it's best to wait for the jdk10 repo to open.
I've checked with Jesper and we can/should quarantine this test right away. I'll take care of it [1]. Best regards, Tobias [1] http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-December/02...
Hi Sherman, On 14/12/2017 10:01 AM, Xueming Shen wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now.
It would be better to just add the test to the ProblemList.txt IMHO. Thanks, David
thanks, Sherman
On 14/12/2017 00:01, Xueming Shen wrote:
Hi
Please help review the change for JDK-8193479
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev
The internal interface ArrayEn/Decoder for ISO8859_1 has been removed because it is no longer used by StringCoding class, but it appears it is being used by hotspot Test6896617.java to verify the SSE instructions on x86. The proposed change here is to simply restore the internal interface ArrayEn/Decoder for ISO8859_1 to avoid blocking hotspot testing for now. I can't tell if this update makes use of the SSE instructions or not.
Would it be better to just exclude the test by adding to the ProblemList.txt and create a hotspot/compiler bug to get it updated to work with the new implementation? -Alan
It appears the consensus is to put this one into the ProblemList and let the hotspot folks to fix the test case. issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev The corresponding hotspot/compiler issue filed: JDK-8193549. I would assume now it needs to get into 11 and backport to 10 repo. Thanks, Sherman
+1 On 12/14/2017 12:51 PM, Xueming Shen wrote:
It appears the consensus is to put this one into the ProblemList and let the hotspot folks to fix the test case.
issue: https://bugs.openjdk.java.net/browse/JDK-8193479 webrev: http://cr.openjdk.java.net/~sherman/8193479/webrev
The corresponding hotspot/compiler issue filed: JDK-8193549.
I would assume now it needs to get into 11 and backport to 10 repo.
Thanks, Sherman
participants (7)
-
Alan Bateman
-
David Holmes
-
Martin Buchholz
-
Roger Riggs
-
Tobias Hartmann
-
Vladimir Kozlov
-
Xueming Shen