JDK-8230459: Test failed to resume JVMCI CompilerThread

[David Holmes]

On 28/10/2019 1:42 pm, Kim Barrett wrote:
>> On Oct 27, 2019, at 6:04 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 24/10/2019 12:47 am, Doerr, Martin wrote:
>>> Hi Kim,
>>> I didn't like using the OopStorage stuff directly, either. I just have not seen how to allocate a global handle and add the oop later.
>>> Thanks for pointing me to JVMCI::make_global. I was not aware of that.
>>> So I can imagine 3 ways to implement it:
>>> 1. Use JNIHandles::destroy_global to release the handles. I just added that to
>>> http://cr.openjdk.java.net/~mdoerr/8230459_JVMCI_thread_objects/webrev.01/
>>> We may want to improve that further by setting the handle pointer to NULL and asserting that it is NULL before adding the new one.
>>> I had been concerned about NULLs in the array, but looks like the existing code can deal with that.
>>
>> I think it would be cleaner to both destroy the global handle and NULL it in the array at the same time.
>>
>> This comment
>>
>> 325         // Old j.l.Thread object can die here.
>>
>> Isn't quite accurate. The j.l.Thread is still referenced via ct->threadObj() so can't "die" until that is also cleared during the actual termination process.
> 
> I think if there is such a thread here that it can't die, because the
> death predicate (the can_remove stuff) won't see that old thread as
> the last thread in _compiler2_objects.  That's what I meant by this:
> 
>> On Oct 25, 2019, at 4:05 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> I also think that here:
>>
>> 947         jobject thread_handle = JNIHandles::make_global(thread_oop);
>> 948         _compiler2_objects[i] = thread_handle;
>>
>> should assert _compiler2_objects[i] == NULL.  Or if that isn't a valid
>> assertion then I think there are other problems.
> 
> I think either that comment about an old thread is wrong (and the NULL
> assertion I suggested is okay), or I think the whole mechanism here
> has problems.  Or at least I was unable to figure out how it could work...
> 

I'm not following sorry. You can't assert NULL unless it's actually set 
to NULL which it presently isn't. But it could be set NULL as Martin 
suggested:

"We may want to improve that further by setting the handle pointer to 
NULL and asserting that it is NULL before adding the new one."

and which I also supported. But that aside once the delete_global has 
been called that JNIHandle no longer references the j.l.Thread that it 
did, at which point it is only reachable via the threadObj() of the 
CompilerThread.

David