On Tue, 5 Jan 2021 14:48:13 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> This change allows JRT_LEAF functions to create Handles because it doesn't
>> need them but some code inside the VM needs Handles. There's a
>> NoSafepointVerifier in JRT_LEAF functions.
>> The JNI_LEAF and JVM_LEAF functions have NoHandleMark (so you can't create
>> handles) but the transition ThreadInVMfromNative will reenable the
>> HandleMarks, so that all this code doesn't have to do it itself.
>> Tested with tier1-6.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix copyrights.
So these changes boil down to:
- remove ResetNoHandleMark and NoHandleMark helpers in favor of having them in
the appropriate interfaceSupport.inline.hpp macros and helpers
- ThreadInVMfromNative now has a ResetNoHandleMark helper
- VM_LEAF_BASE no longer needs a NoHandleMark helper
- VM_ENTRY_BASE_FROM_LEAF no longer needs a ResetNoHandleMark helper
- JNI_LEAF now has a NoHandleMark helper
- JVM_LEAF now has a NoHandleMark helper
- JVM_ENTRY_FROM_LEAF now has a ResetNoHandleMark helper
Okay, I think I grok these changes and they look good to me.
I would like to see Tier7 and Tier8 testing done since longer
stress runs are done in those tiers.
src/hotspot/share/c1/c1_Runtime1.cpp line 647:
> 645: address pc = thread->exception_pc();
> 646: // Still in Java mode
> 647: nmethod* nm = NULL;
You are definitely going to want a C1 cognizant person to
review the changes in this file. The baseline's use of
ResetNoHandleMark and NoHandleMark in this file is an
absolute mess.
I don't understand the reason for old L647 here and for
old L652 below. Double ResetNoHandleMarks, but why?
I see no reason for old L647 so glad that it's gone!
src/hotspot/share/c1/c1_Runtime1.cpp line 1311:
> 1309: patch_code(thread, load_klass_patching_id);
> 1310:
> 1311: // Back in JAVA, use no oops DON'T safepoint
The baseline here is even more confused.
debug_only NoHandleMark here on old L1311
and a ResetNoHandleMark on old L1315.
I see no reason for old L1311 so glad that it's gone!
src/hotspot/share/c1/c1_Runtime1.cpp line 1330:
> 1328: }
> 1329:
> 1330: int Runtime1::move_appendix_patching(JavaThread* thread) {
And again a debug_only NoHandleMark on old L1330
and ResetNoHandleMark on old L1334. What the heck?
src/hotspot/share/c1/c1_Runtime1.cpp line 1349:
> 1347: // helper method which does the normal VM transition and when it
> 1348: // completes we can check for deoptimization. This simplifies the
> 1349: // assembly code in the cpu directories.
And again a debug_only NoHandleMark on old L1349
and ResetNoHandleMark on old L1353. What the heck?
src/hotspot/share/c1/c1_Runtime1.cpp line 1376:
> 1374: // between the C calling convention and the Java one.
> 1375: // e.g., on x86, GCC may clear only %al when returning a bool false,
> but
> 1376: // JVM takes the whole %eax as the return value, which may
> misinterpret
And again a debug_only NoHandleMark on old L1376
and ResetNoHandleMark on old L1380. What the heck?
src/hotspot/share/jvmci/jvmciRuntime.cpp line 375:
> 373: address pc = thread->exception_pc();
> 374: // Still in Java mode
> 375: CompiledMethod* cm = NULL;
I don't understand the reason for old L375 here and for
old L380 below. Double ResetNoHandleMarks, but why?
I see no reason for old L375 so glad that it's gone!
src/hotspot/share/opto/runtime.cpp line 1362:
> 1360: SharedRuntime::_find_handler_ctr++; // find exception handler
> 1361: #endif
> 1362: nmethod* nm = NULL;
The baseline here is even more confused.
debug_only NoHandleMark here on old L1362
and a ResetNoHandleMark on old L1368.
I see no reason for old L1362 so glad that it's gone!
src/hotspot/share/runtime/deoptimization.cpp line 153:
> 151:
> 152: // In order to make fetch_unroll_info work properly with escape
> 153: // analysis, The method was changed from JRT_LEAF to JRT_BLOCK_ENTRY.
nit - typo (not yours): s/, The/, the/
src/hotspot/share/runtime/handles.hpp line 292:
> 290:
> 291: // ResetNoHandleMark is called in a context where there is an enclosing
> 292: // NoHandleMark. Thread in _thread_in_native cannot create handles so
Perhaps:
// NoHandleMark. A thread in _thread_in_native must not create handles so
src/hotspot/share/runtime/handles.hpp line 293:
> 291: // ResetNoHandleMark is called in a context where there is an enclosing
> 292: // NoHandleMark. Thread in _thread_in_native cannot create handles so
> 293: // this is used when transitioning via. ThreadInVMfromNative.
nit typo: s/via./via/
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1846
