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

[coleen.phillimore at oracle.com]

On 8/30/19 2:56 AM, Robbin Ehn wrote:
> Hi David, thanks for having a look!
>
> On 2019-08-30 03:24, David Holmes wrote:
>> Hi Robbin,
>>
>> On 29/08/2019 10:48 pm, Robbin Ehn wrote:
>>> Hi all, please review.
>>>
>>> The JNI_QUICK_ENTRY/JVM_QUICK_ENTRY skips creating the 
>>> HandleMarkCleaner, instead they have a NoHandleMark. Handshakes can
>>> be executed by the JavaThread itself, thus might require a
>>> HandleMark. QUICK entries allow polls for safepoint/handshakes, if a
>>> handshake is executed and a HandleMark is created we hit this
>>> assert.
>>>
>>> HandleMarkCleaner have previous been optimized for inlining, thus 
>>> cheap.
>>> Performance benchmarks shows no benefit of not allowing HandleMark.
>>>
>>> This changeset removes the the QUICK entry and avoids this issue.
>>>
>>> Note that JNI_QUICK_ENTRY also don't have WeakPreserveExceptionMark,
>>> so it's changed to JNI_ENTRY_NO_PRESERVE.
>>
>> Why did you extend this to the JNI_QUICK_ENTRY case when the impacted 
>> code was only a JVM_QUICK_ENTRY? The expanded scope of the change 
>> seems unnecessary given this is just a workaround for a particular 
>> failure mode.
>
> - If your code require you not to use any HandleMarks it most likely 
> require no
> polls, for correctness. JRT_LEAF does this correctly, sets up both 
> NoSafepointVerifier and NoHandleMark. (with the HandleMark in 
> handshake we now always require NoSafepointVerifier if you use 
> NoHandleMark for correctness)
>
> - There is no performance benefit, so we just have extra code which is 
> useless.
>
> - It might be affected, I have not track down every path. But even if 
> it's
> not, nothing is stopping someone from accidentally changing the code 
> to call
> something which does a poll, since we don't have NoSafepointVerifier.
> So we avoid getting the same failure mode in future.
>
> - Reducing the complexity of interafceSupport is always good!

Please!
>
> - There was no reason to keep it.
>
> So if you don't a very strong opinion about it, I really like to get 
> rid of it.

I'd really like for you to get rid of it too because it doesn't make 
sense, and I've already wasted too much of my time to try to figure out 
that it doesn't make sense.   More below.

Your change looks good to me.  There's a VM_QUICK_ENTRY_MARK that's 
broken/similar in share/ci/ciUtilities that should be cleaned up too but 
you can file another RFE for it.

>
> 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.
>

Looking through the code, I've come to the same conclusion.  I'm pretty 
sure most of it can be removed, in favor of adding an assert that the 
JavaThread state is _thread_in_vm in the Handle constructor.   Also a 
lot of these NoHandleMark and ResetNoHandleMarks are leftover from 
permgen when metadata needed to be handled.  They don't seem to apply to 
oops in the places where they are.  I think we should file some RFEs to 
clean them up.

Thanks,
Coleen

>
>>
>> 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