RFR: 8290370: Convert SymbolPropertyTable to ResourceHashtable [v3]

[Ioi Lam]

On Thu, 21 Jul 2022 19:58:51 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Justin Gu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix whitespaces
>
> src/hotspot/share/classfile/systemDictionary.cpp line 2023:
> 
>> 2021:                                                        TRAPS) {
>> 2022: 
>> 2023:   MutexLocker ml(THREAD, InvokeMethodTable_lock);
> 
> Note to reviewers: holding this lock throughout creating the MethodHandle intrinsic, prevents us from leaking a Method* that the code cache is pointing to, so is not easily removed.

Since this is a refactoring RFE, I think we should avoid substantial logic changes, especially ones related to locking. 

If we hold the `InvokeMethodTable_lock` lock the whole time, it will prevent two concurrent threads from calling `Method::make_method_handle_intrinsic` on two different `iid`. This seems to be an optimization that the original code tries to do (or unintentionally allows), with the trade off being that if two threads try to use the same ``iid`, we might make two method handles and throw away one of them. The unused one might be garbage collected though. Without looking at the details, I can't confirm that a leak would happen, or if it does, how often.

In any case, if we think that such a change is necessary, it should be deferred to a follow-up PR.

If I understand correctly, the original code can be done without a lock, because the old Hashtable allows lock-free reads, but ResourceHashtable doesn't.


SymbolPropertyEntry* spe = invoke_method_table()->find_entry(index, hash, signature, iid_as_int);


I would suggest changing the new code to do the same two-step locking as done in the new version of `SystemDictionary::find_method_handle_type()`, but you might need to hold `SystemDictionary_lock` instead. Please see my comments for `SystemDictionary::methods_do()`.

> src/hotspot/share/runtime/java.cpp line 117:
> 
>> 115: 
>> 116: void print_method_profiling_data() {
>> 117:   if ((ProfileInterpreter COMPILER1_PRESENT(|| C1UpdateMethodData)) &&
> 
> Note to reviewers: This missing () made us iterate over methods at every exit for non-product mode.

What's the performance impact? Should we backport this single line change to older releases? If so, it may be better to put this change in a separate PR to keep the history clean.

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

PR: https://git.openjdk.org/jdk/pull/9495