RFR(s): 8228758: assert(_no_handle_mark_nesting == 0) failed: allocating handle inside NoHandleMark

[Daniel D. Daugherty]

Just jumping in with a comment on one point below...

On 9/3/19 8:43 AM, Robbin Ehn wrote:
> Hi David,
>
> Truncated:
>
>>
>> You stated that if you need to not use any handleMarks then you most 
>> likely don't want polls - and no polls is what a LEAF routine has to 
>> guarantee:
>>
>> QUICK -> No handles -> no polls == LEAF
>
> I'll look at the other way around:
> QUICK can have polls therefore they cannot be leafs.
> (at least the one triggering this assert)
>
>>
>>> Since we missing the cleaner, accidentally using a HandleMark would 
>>> lead to a leak. The NoHandleMark protected against this leak.
>>
>> Maybe I'm misunderstanding exactly how HandleMarks nest and behave. I 
>> would have thought this nesting would be allowed:
>>
>> entry_point
>>    NoHandleMark
>>    -> subroutine
>>       HandleMark  // prevents leak of Handle to outer scope
>>       allocate handle
>>
>> whereas this would cause an error
>>
>> entry_point
>>    NoHandleMark
>>    -> subroutine
>>       allocate handle
>>
>> but IIUC a NoHandleMark prevents all and any handle use in its own 
>> scope or a nest scope - no?
>
> Yes, sorry, NoHandleMark increments _no_handle_mark_nesting and
> HandleArea::allocate_handle checks that it is 0, otherwise this assert.
> (it also checks that _handle_mark_nesting is larger than 0, so it must 
> be inside a HandleMark)
>
>>
>> I'm not sure that is viable, or the right way to try and address 
>> this. In the extreme it would require all VM susbsystems to be fully 
>> re-entrant and that is not a reasonable expectation.
>
> We already have that today with safepoint, many parts of subsystems 
> can't handle
> safepoints. The one adding the new code to be run in a handshake must 
> make sure
> that works. Not saying we should be able to run anything in a 
> handshake, but
> fundamental things like Handles should be allowed.

I'm going to have to disagree with the comparison here. I don't think
comparing handshakes with blocking for safepoints is quite right. A
thread blocking for a safepoint while executing a subsystem's code is
a synchronous thing in my mind:

     I'm calling into the subsystem and that subsystem uses locks so my
     thread might safepoint while in there...

     The subsystem has to synchronously prepare itself for grabbing that
     lock by Handle-izing oops, etc...

When I think about handshakes, I think about them as happening 
asynchronously
like async-signals resulting in calls to handlers... Maybe my mental 
model of
handshakes is wrong... :-)

In any case, I'm going to hold off on reviewing the code since I think
you're still considering making changes based on David's last set of
comments...

Dan



>
>>
>>> This changes the polling model a bit and we have not proper look at 
>>> this.
>>> Maybe we should add the constraints to the poll and to find any 
>>> potential
>>> troublesome polls.
>>
>> I'm not sure how we would discover the problems until we have a 
>> handshake that executes some code that violates a constraint. The 
>> constraints themselves are not obvious. Subsystems do not clearly 
>> identify themselves as being reentrant or not - most things are not 
>> reentrant by their nature.
>
> As we do with safepoints, NoSafepointVerifier (since it covers both 
> cases).
>
> /Robbin
>
>>
>> Cheers,
>> David
>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> Thanks, Robbin
>>>>>
>>>>> Some extra words about NoHandleMark:
>>>>> I believe NoHandleMark should be removed.
>>>>> In the LEAF case, there is no point in creating a Handle within a
>>>>> NoSafepointVerifier, since we don't safepoint.
>>>>> The other usecase seems to be, I'm in java don't do VM things, and 
>>>>> someone tried
>>>>> to protected that with a NoHandleMark. There many ways to create a 
>>>>> much more
>>>>> robust check for that.
>>>>> But a discussion for another time.
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Issue:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8228758
>>>>>>>
>>>>>>> Changeset:
>>>>>>> http://cr.openjdk.java.net/~rehn/8228758/webrev/
>>>>>>>
>>>>>>> Passes t1-3 (and performance benchmarks)
>>>>>>>
>>>>>>> Thanks, Robbin