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

[David Holmes]

Hi Robbin,

On 30/08/2019 4:56 pm, 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)

I think the distinction between QUICK and LEAF is very murky. But you 
seem to be suggesting an equivalence that would then lead to replacement 
of QUICK entry with a LEAF entry ??

> - There is no performance benefit, so we just have extra code which is 
> useless.

I tried to get some background on the QUICK_ENTRY and why the HandleMark 
removal made it "quick", but this is day one code from Oct 1998 and 
there is no explanation I can find. I also find the comments a bit 
misleading:

// QUICK_ENTRY routines behave like ENTRY but without a handle mark

This suggests that having a HandleMark somehow slows things down so it 
is removed. But it doesn't just remove the HandleMark (or these days 
HandleMarkCleaner) from the entry, it also adds a NoHandleMark, which is 
asserting quite a different constraint.

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

I could argue that it would be useful to see such a failure during 
testing of such a change to realize that it doesn't in fact introduce a 
poll path.

> - Reducing the complexity of interafceSupport is always good!

Can't argue there :)

> - 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 don't have a strong enough opinion to fight to keep it.

But I remain very concerned that the true problem here is that 
handshakes can now lead to executing code that violates the expectations 
of code at the poll site. We used to know exactly what code could be 
executed in a given path, but now it will expand to include the set of 
in-thread handshake operations that may be possible. For now that seems 
restricted to biased-locking revocation, but the use of Handshakes is 
set to expand.

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