Quantcast

RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Aleksey Shipilev-4
Hi,

Seeing failures on new jcstress runs:
  http://openjdk.linaro.org/jdk9/jcstress-nightly-runs/2017/056/results/

These "opaque" tests should never fail. I eyeballed the test, and this is not a
test bug.

C1 does not have intrinsics for Opaque, and therefore it should delegate Opaque
ops to volatile via fallbacks in Unsafe.java. But now I read the C1
Unsafe.getObject handling, and my hair stand on end, because I think C1 {L,G}VN
does not treat volatile Unsafe ops any specially, while it probably should:
 https://bugs.openjdk.java.net/browse/JDK-8175887

Makes sense?

Thanks,
-Aleksey

P.S. Linaro CI seems misconfigured by always running with C1, so can't see if
that is C1 specific or not:
http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-February/004246.html


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
On 02/27/2017 10:53 AM, Aleksey Shipilev wrote:
> C1 does not have intrinsics for Opaque, and therefore it should delegate Opaque
> ops to volatile via fallbacks in Unsafe.java. But now I read the C1
> Unsafe.getObject handling, and my hair stand on end, because I think C1 {L,G}VN
> does not treat volatile Unsafe ops any specially, while it probably should:
>  https://bugs.openjdk.java.net/browse/JDK-8175887
>
> Makes sense?

This is the actual reproducible bug, changing the thread subject.

Fix against jdk9/hs:
  http://cr.openjdk.java.net/~shade/8175887/webrev.01/

Testing: Linux x86_64/release hotspot/test/compiler, includes new tests

I need a sponsor. I think this bugfix is too important to miss 9, and it even
deserves backporting to 8u.

Thanks,
-Aleksey






signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Stuart Monteith
In reply to this post by Aleksey Shipilev-4
Hi,
        I've run this particular test with no options manually and a build from
today and see the following:


Will throw any pending exceptions at this point.
Exception in thread "main" java.lang.AssertionError: TEST FAILURES:

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
-XX:+StressGCM]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-client]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
-XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0

Applying your patch and rerunning this test on the aarch64 machine
causes these tests to pass. Now doing a full run. It should be finished
by Tuesday.

BR,
   Stuart


On 27/02/17 09:53, Aleksey Shipilev wrote:

> Hi,
>
> Seeing failures on new jcstress runs:
>   http://openjdk.linaro.org/jdk9/jcstress-nightly-runs/2017/056/results/
>
> These "opaque" tests should never fail. I eyeballed the test, and this is not a
> test bug.
>
> C1 does not have intrinsics for Opaque, and therefore it should delegate Opaque
> ops to volatile via fallbacks in Unsafe.java. But now I read the C1
> Unsafe.getObject handling, and my hair stand on end, because I think C1 {L,G}VN
> does not treat volatile Unsafe ops any specially, while it probably should:
>  https://bugs.openjdk.java.net/browse/JDK-8175887
>
> Makes sense?
>
> Thanks,
> -Aleksey
>
> P.S. Linaro CI seems misconfigured by always running with C1, so can't see if
> that is C1 specific or not:
> http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-February/004246.html
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Aleksey Shipilev-4
Hi Stuart,

On 02/27/2017 03:32 PM, Stuart Monteith wrote:

> Will throw any pending exceptions at this point.
> Exception in thread "main" java.lang.AssertionError: TEST FAILURES:
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
> -XX:+StressGCM]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-client]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
> -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0
>
> Applying your patch and rerunning this test on the aarch64 machine
> causes these tests to pass. Now doing a full run. It should be finished
> by Tuesday.
So Opaque failures on your machines *are* related to this. Good to know, thanks!

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Aleksey Shipilev-4
On 02/27/2017 03:35 PM, Aleksey Shipilev wrote:

> On 02/27/2017 03:32 PM, Stuart Monteith wrote:
>> Will throw any pending exceptions at this point.
>> Exception in thread "main" java.lang.AssertionError: TEST FAILURES:
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
>> -XX:+StressGCM]: Observed forbidden state: -1, 0
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-server]: Observed forbidden state: -1, 0
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-client]: Observed forbidden state: -1, 0
>>
>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>> [-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
>> -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0
>>
>> Applying your patch and rerunning this test on the aarch64 machine
>> causes these tests to pass. Now doing a full run. It should be finished
>> by Tuesday.
>
> So Opaque failures on your machines *are* related to this. Good to know, thanks!
It does not sound right though: with -TieredCompilation only C2 should have been
working, not C1. Must be something else.

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Stuart Monteith
Actually, rerunning without any jvmargs (which reduced the coverage when
testing your patch), I get:

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
-XX:+StressGCM]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
-XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0

org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
[-client]: Observed forbidden state: -1, 0

So what I said before was wrong, I do get the failures.
Running with -XX:TieredStopAtLevel=1, I don't get failures.

Given that -server == -client, all of the failing tests are either
against C2 or tiered C1 & C2 compilation.

Running with the options passed explicity, "-m tough", the tests pass with:
        -XX:TieredStopAtLevel=1   (Patched & unpatched)

Fails with:
        -XX:-TieredCompilation              "

No failures with:
        -XX:-TieredComplation -Xcomp

My other two machines doen't exhibit the failure.

BR,
        Stuart



On 27/02/17 14:39, Aleksey Shipilev wrote:

> On 02/27/2017 03:35 PM, Aleksey Shipilev wrote:
>> On 02/27/2017 03:32 PM, Stuart Monteith wrote:
>>> Will throw any pending exceptions at this point.
>>> Exception in thread "main" java.lang.AssertionError: TEST FAILURES:
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
>>> -XX:+StressGCM]: Observed forbidden state: -1, 0
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-server]: Observed forbidden state: -1, 0
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-client]: Observed forbidden state: -1, 0
>>>
>>> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
>>> [-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
>>> -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0
>>>
>>> Applying your patch and rerunning this test on the aarch64 machine
>>> causes these tests to pass. Now doing a full run. It should be finished
>>> by Tuesday.
>>
>> So Opaque failures on your machines *are* related to this. Good to know, thanks!
>
> It does not sound right though: with -TieredCompilation only C2 should have been
> working, not C1. Must be something else.
>
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFC 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect?

Aleksey Shipilev-4
On 02/27/2017 04:21 PM, Stuart Monteith wrote:

> Actually, rerunning without any jvmargs (which reduced the coverage when
> testing your patch), I get:
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-client, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM,
> -XX:+StressGCM]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:-TieredCompilation, -XX:+UnlockDiagnosticVMOptions,
> -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-server, -XX:-TieredCompilation]: Observed forbidden state: -1, 0
>
> org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest
> [-client]: Observed forbidden state: -1, 0
>
> So what I said before was wrong, I do get the failures.
> Running with -XX:TieredStopAtLevel=1, I don't get failures.
Good. This is what I wanted to know for this patch.

> Given that -server == -client, all of the failing tests are either
> against C2 or tiered C1 & C2 compilation.
>
> Running with the options passed explicity, "-m tough", the tests pass with:
> -XX:TieredStopAtLevel=1   (Patched & unpatched)
>
> Fails with:
> -XX:-TieredCompilation              "
>
> No failures with:
> -XX:-TieredComplation -Xcomp
>
> My other two machines doen't exhibit the failure.
Ok, let's discuss that one separately. These failures seem unrelated to this
issue with C1, and most probably indicate some other peculiar issue.

Thanks,
-Aleksey

Thanks,
-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
In reply to this post by Aleksey Shipilev-4
On 02/27/2017 03:12 PM, Aleksey Shipilev wrote:

> On 02/27/2017 10:53 AM, Aleksey Shipilev wrote:
>> C1 does not have intrinsics for Opaque, and therefore it should delegate Opaque
>> ops to volatile via fallbacks in Unsafe.java. But now I read the C1
>> Unsafe.getObject handling, and my hair stand on end, because I think C1 {L,G}VN
>> does not treat volatile Unsafe ops any specially, while it probably should:
>>  https://bugs.openjdk.java.net/browse/JDK-8175887
>>
>> Makes sense?
>
> This is the actual reproducible bug, changing the thread subject.
>
> Fix against jdk9/hs:
>   http://cr.openjdk.java.net/~shade/8175887/webrev.01/
Updated after off-list review (renamed tests, used internal Unsafe):
  http://cr.openjdk.java.net/~shade/8175887/webrev.02/

Everything else still stands:

> Testing: Linux x86_64/release hotspot/test/compiler, includes new tests
>
> I need a sponsor. I think this bugfix is too important to miss 9, and it even
> deserves backporting to 8u.

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Paul Sandoz

> On 27 Feb 2017, at 21:25, Aleksey Shipilev <[hidden email]> wrote:
>
> On 02/27/2017 03:12 PM, Aleksey Shipilev wrote:
>> On 02/27/2017 10:53 AM, Aleksey Shipilev wrote:
>>> C1 does not have intrinsics for Opaque, and therefore it should delegate Opaque
>>> ops to volatile via fallbacks in Unsafe.java. But now I read the C1
>>> Unsafe.getObject handling, and my hair stand on end, because I think C1 {L,G}VN
>>> does not treat volatile Unsafe ops any specially, while it probably should:
>>> https://bugs.openjdk.java.net/browse/JDK-8175887
>>>
>>> Makes sense?
>>
>> This is the actual reproducible bug, changing the thread subject.
>>
>> Fix against jdk9/hs:
>>  http://cr.openjdk.java.net/~shade/8175887/webrev.01/
>
> Updated after off-list review (renamed tests, used internal Unsafe):
>  http://cr.openjdk.java.net/~shade/8175887/webrev.02/
>
This looks good, with my limited knowledge of C1 (but expanded with some help from Aleksey on explaining GVN), but need someone from the compiler team to formally review.

Paul.

> Everything else still stands:
>
>> Testing: Linux x86_64/release hotspot/test/compiler, includes new tests
>>
>> I need a sponsor. I think this bugfix is too important to miss 9, and it even
>> deserves backporting to 8u.
>
> -Aleksey
>


signature.asc (858 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov
In reply to this post by Aleksey Shipilev-4
Thanks for the fix, Aleksey.

> Updated after off-list review (renamed tests, used internal Unsafe):
>   http://cr.openjdk.java.net/~shade/8175887/webrev.02/

I'm fine with the fix in do_UnsafeGetObject.

Changes in do_UnsafeGetRaw don't make much sense to me. Following the
reasoning in the comment ("better be safe than sorry") you have to
unconditionally kill memory for UnsafeGetObject as well :-)

If there's a need in ordering raw loads, I'd prefer to see a dedicated
flag (like UnsafeObjectOp::_is_volatile) introduced instead.
Right now, UnsafeGetRaw usage is very limited (only for restoring frame
state in OSR entry), so I don't see any reason in doing that.

So, please, leave it as is.

Best regards,
Vladimir Ivanov

> Everything else still stands:
>
>> Testing: Linux x86_64/release hotspot/test/compiler, includes new tests
>>
>> I need a sponsor. I think this bugfix is too important to miss 9, and it even
>> deserves backporting to 8u.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
On 02/28/2017 04:28 PM, Vladimir Ivanov wrote:

> Thanks for the fix, Aleksey.
>
>> Updated after off-list review (renamed tests, used internal Unsafe):
>>   http://cr.openjdk.java.net/~shade/8175887/webrev.02/
>
> I'm fine with the fix in do_UnsafeGetObject.
>
> Changes in do_UnsafeGetRaw don't make much sense to me. Following the reasoning
> in the comment ("better be safe than sorry") you have to
> unconditionally kill memory for UnsafeGetObject as well :-)
>
> If there's a need in ordering raw loads, I'd prefer to see a dedicated flag
> (like UnsafeObjectOp::_is_volatile) introduced instead.
> Right now, UnsafeGetRaw usage is very limited (only for restoring frame state in
> OSR entry), so I don't see any reason in doing that.
>
> So, please, leave it as is.
All right, fine:
  http://cr.openjdk.java.net/~shade/8175887/webrev.03/

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov
> All right, fine:
>   http://cr.openjdk.java.net/~shade/8175887/webrev.03/

Thanks, looks good.

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov


On 2/28/17 7:42 PM, Vladimir Ivanov wrote:
>> All right, fine:
>>   http://cr.openjdk.java.net/~shade/8175887/webrev.03/
>
> Thanks, looks good.

FYI spotted a couple of issues in the tests. Once the testing is over I
plan to push the following version (w/ some cleanups):
  http://cr.openjdk.java.net/~vlivanov/shade/8175887/

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
On 02/28/2017 08:06 PM, Vladimir Ivanov wrote:
> On 2/28/17 7:42 PM, Vladimir Ivanov wrote:
>>> All right, fine:
>>>   http://cr.openjdk.java.net/~shade/8175887/webrev.03/
>>
>> Thanks, looks good.
>
> FYI spotted a couple of issues in the tests. Once the testing is over I plan to
> push the following version (w/ some cleanups):
>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/

The cleanup look good, but are you sure AssertionError would get reported from
the thread, and it would not silently die? The original test exited hard to
handle this.

Thanks,
-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov

>>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/
>
> The cleanup look good, but are you sure AssertionError would get reported from
> the thread, and it would not silently die? The original test exited hard to
> handle this.

Good point. Reverted that part. (Updated the webrev in place.)

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
On 02/28/2017 10:01 PM, Vladimir Ivanov wrote:
>
>>>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/
>>
>> The cleanup look good, but are you sure AssertionError would get reported from
>> the thread, and it would not silently die? The original test exited hard to
>> handle this.
>
> Good point. Reverted that part. (Updated the webrev in place.)

Thumbs up. Thanks for handling this!

-Aleksey


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov
Pushed:
   http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/2ff05d967fb2

Best regards,
Vladimir Ivanov

On 3/1/17 12:01 AM, Aleksey Shipilev wrote:

> On 02/28/2017 10:01 PM, Vladimir Ivanov wrote:
>>
>>>>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/
>>>
>>> The cleanup look good, but are you sure AssertionError would get reported from
>>> the thread, and it would not silently die? The original test exited hard to
>>> handle this.
>>
>> Good point. Reverted that part. (Updated the webrev in place.)
>
> Thumbs up. Thanks for handling this!
>
> -Aleksey
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Aleksey Shipilev-4
Hi again,

If you see no troubles in nightly runs, can you propose this to 8u?
I can propose myself, if you want.

-Aleksey

On 03/01/2017 02:41 PM, Vladimir Ivanov wrote:

> Pushed:
>   http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/2ff05d967fb2
>
> Best regards,
> Vladimir Ivanov
>
> On 3/1/17 12:01 AM, Aleksey Shipilev wrote:
>> On 02/28/2017 10:01 PM, Vladimir Ivanov wrote:
>>>
>>>>>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/
>>>>
>>>> The cleanup look good, but are you sure AssertionError would get reported from
>>>> the thread, and it would not silently die? The original test exited hard to
>>>> handle this.
>>>
>>> Good point. Reverted that part. (Updated the webrev in place.)
>>
>> Thumbs up. Thanks for handling this!
>>
>> -Aleksey
>>


signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (S) 8175887: C1 value numbering handling of Unsafe.get*Volatile is incorrect

Vladimir Ivanov
Feel free to send approval request. Nightlies look clean.

Best regards,
Vladimir Ivanov

On 3/2/17 11:08 PM, Aleksey Shipilev wrote:

> Hi again,
>
> If you see no troubles in nightly runs, can you propose this to 8u?
> I can propose myself, if you want.
>
> -Aleksey
>
> On 03/01/2017 02:41 PM, Vladimir Ivanov wrote:
>> Pushed:
>>   http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/2ff05d967fb2
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 3/1/17 12:01 AM, Aleksey Shipilev wrote:
>>> On 02/28/2017 10:01 PM, Vladimir Ivanov wrote:
>>>>
>>>>>>  http://cr.openjdk.java.net/~vlivanov/shade/8175887/
>>>>>
>>>>> The cleanup look good, but are you sure AssertionError would get reported from
>>>>> the thread, and it would not silently die? The original test exited hard to
>>>>> handle this.
>>>>
>>>> Good point. Reverted that part. (Updated the webrev in place.)
>>>
>>> Thumbs up. Thanks for handling this!
>>>
>>> -Aleksey
>>>
>
Loading...