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

[Robbin Ehn]

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!

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

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