RFR(S) [15] : 8208207 : Test nsk/stress/jni/gclocker/gcl001 fails after co-location

[Thomas Schatzl]

Hi,

On 30.06.20 05:23, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev//8208207/webrev.00
>> 38 lines changed: 1 ins; 4 del; 33 mod;
> 
> Hi all,
> 
> could you please review the small patch which fixes gcl001 test and returns it back to work?
> 
> the issue reported in the bug (assert(!JavaThread::current()->in_critical()) failed: Would deadlock) was caused by calls to JNI functions other than (Get|Release).*Critical within a critical region. after this get fixed by moving Get.*Length calls outside of critical regions, the test started to fail w/ "Data validation failure" message. this was b/c of returning 0 and w/o sorting arrays in case isCopy was changed to TRUE by Get.*Critical. as there is no way to guarantee that we won't get a copy of array, I decided to remove checks of isCopy which, although might slightly change that this test check, makes the test more robust.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8208207
> webrev: http://cr.openjdk.java.net/~iignatyev//8208207/webrev.00
> testing:
>   - run the test against {linux,windows,macosx-x64}-{product,fastdebug} w/ default GC
>   - run the test against macosx-x64-slowdebug w/ Serial,Parallel,G1,ZGC
> 

- I would prefer if the test removed the debug code, i.e. the native 
EnterCS/ReleaseCS methods and associated data structures. At least 
instead of commenting out code to disable it, add an ifdef.

- could the backslashes in the macro be lined up? Otherwise the code is 
even uglier than it already is :P

- unless it is intentional, the code would be easier to read if the 
GetxxxCritical and ReleasexxxCritical were scoped better, i.e. now it is 
like:

GetPrimitiveArrayCritical
GetStringCritical
ReleaseStringCritical
GetStringCritical
ReleasePrimitiveArrayCritical <--- this one
ReleaseStringCritical

and the question is why the ReleasePrimitiveArrayCritical is between the 
second get/releaseStringCritical and not the last call.

Thanks,
   Thomas