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

[Igor Ignatyev]

Hi Thomas,

please find the answers to your questions inlined below. 

I've made the cleanup you asked and some other changes to make the code, hopefully, nicer. I've also removed zeroing of hash in native part and updated java accordingly, so the test would fail if 1st native loop sees some other data.
 
 - incremental webrev: http://cr.openjdk.java.net/~iignatyev//8208207/webrev.0-1
 - full webrev: http://cr.openjdk.java.net/~iignatyev//8208207/webrev.0

Thanks,
-- Igor

>> On Jun 30, 2020, at 2:50 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>> 
>> 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.
> at 1st, I removed EnterCS/LeaveCS methods and two vars, but then restored them back just to keep the change as small as possible, a part of me was still disgusted by that, yet given the code around it was hard to pinpoint the reason for that :)  anyhow, I've cleaned that up.
> 
> - could the backslashes in the macro be lined up?
sure.
> Otherwise the code is even uglier than it already is :P
is it even possible ;) ?
> 
> - 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.
I'm not an original author of this test, but the current order does seem intentional, so I'd prefer not to change that.


> 
> Thanks,
>  Thomas