RFR: 8263377: Store method handle linkers in the 'non-nmethods' heap [v2]

[Yi-Fan Tsai]

On Fri, 3 Jun 2022 17:42:00 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Yi-Fan Tsai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove dead codes
>>   
>>   remove unused argument of NativeJump::check_verified_entry_alignment
>>   remove unused argument of NativeJumip::patch_verified_entry
>>   remove dead codes in SharedRuntime::generate_method_handle_intrinsic_wrapper
>
> src/hotspot/share/ci/ciMethod.cpp line 1131:
> 
>> 1129:                      CompiledMethod* cm = (code == nullptr) ? nullptr : code->as_compiled_method_or_null();
>> 1130:                      if (cm != NULL && (cm->comp_level() == CompLevel_full_optimization)) {
>> 1131:                        _instructions_size = cm->insts_end() - cm->verified_entry_point();
> 
> So, the old code only used sets _instruction_size if `comp_level` is `CompLevel_full_optimization`. Since the old shared runtime code used `new_native_nmethod`, which ends up setting comp_level to `CompLevel_none`, this branch is also not being taken in the current code?
> 
> In that case, this looks good.

Correct, comp_level of MH intrinsics were set to `CompLevel_none`.

> src/hotspot/share/code/codeBlob.cpp line 354:
> 
>> 352:   if (mhi != NULL) {
>> 353:     debug_only(mhi->verify();) // might block
>> 354:   }
> 
> This is debug only. Looking at CodeCache::allocate, it can only return `NULL` if the allocation size is `<= 0`, in which case an earlier assert will already fire. So, this null check doesn't seem needed?
> Suggestion:
> 
>   debug_only(mhi->verify();) // might block

This seems needed. CodeCache::allocate may return `NULL` if the code cache is full.

> src/hotspot/share/code/codeBlob.cpp line 360:
> 
>> 358: void MethodHandleIntrinsicBlob::verify() {
>> 359:   // Make sure all the entry points are correctly aligned for patching.
>> 360:   NativeJump::check_verified_entry_alignment(code_begin(), code_begin());
> 
> This method only seems implemented on x86, which ignores the first argument. Maybe it's a good opportunity to clean up the first argument?

Sure. Same for NativeJump::patch_verified_entry.

> src/hotspot/share/compiler/compileBroker.hpp line 307:
> 
>> 305:                                  TRAPS);
>> 306: 
>> 307:   static CodeBlob* compile_method(const methodHandle& method,
> 
> Not so sure about these changes. It seems to me that if a method is requested to be compiled, it should always result in an nmethod.
> 
> Alternatively, would it be possible to keep these functions returning an `nmethod` but add an assert at the start to check that the passed `method` is not a method handle intrinsic?

The implementation assumed that MH intrinsics are possible input. Once the assertion is added, two conditions could be simplified.

> src/hotspot/share/oops/method.cpp line 1304:
> 
>> 1302:     assert(blob->is_mh_intrinsic(), "must be MH intrinsic");
>> 1303:     MethodHandleIntrinsicBlob* mhi = blob->as_mh_intrinsic();
>> 1304:     return (mhi->method() == nullptr) || (mhi->method() == this);
> 
> The assert looks redundant, since the cast on the next line already checks it.
> 
> Also, can `mhi->method()` really be `nullptr` here?

No, it shouldn't be. Removed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8760