Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

Xueming Shen
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 <[hidden email]> 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




Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

David Holmes
Hi Sherman,

On 14/12/2017 12:05 PM, Xueming Shen wrote:
> David, Martin,
>
> webrev has been updated to fix the test directly.
>
> http://cr.openjdk.java.net/~sherman/8193479/webrev

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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

Vladimir Kozlov
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.
>>
>> http://cr.openjdk.java.net/~sherman/8193479/webrev
>
> 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
>>> <[hidden email] <mailto:[hidden email]>> 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
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

Xueming Shen
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.
>>>
>>> http://cr.openjdk.java.net/~sherman/8193479/webrev
>>
>> 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
----------------------------------------------
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

Tobias Hartmann-2
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
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8193479: test/hotspot/jtreg/compiler/codegen/Test6896617.java fails after JDK-8184947

Tobias Hartmann-2
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/027933.html