RFR: 8257189: Handle concurrent updates of MH.form better [v2]

[Vladimir Ivanov]

On Tue, 1 Dec 2020 15:55:36 GMT, Peter Levart <plevart at openjdk.org> wrote:

>> It seems that I was right. See `ciField.cpp`:
>> 
>> static bool trust_final_non_static_fields(ciInstanceKlass* holder) {
>>   if (holder == NULL)
>>     return false;
>>   if (holder->name() == ciSymbol::java_lang_System())
>>     // Never trust strangely unstable finals:  System.out, etc.
>>     return false;
>>   // Even if general trusting is disabled, trust system-built closures in these packages.
>>   if (holder->is_in_package("java/lang/invoke") || holder->is_in_package("sun/invoke") ||
>>       holder->is_in_package("jdk/internal/foreign") || holder->is_in_package("jdk/incubator/foreign") ||
>>       holder->is_in_package("jdk/internal/vm/vector") || holder->is_in_package("jdk/incubator/vector") ||
>>       holder->is_in_package("java/lang"))
>>     return true;
>
> I mean, is it possible that some threads that concurrently use the old uncustomized form while one thread is customizing it, trigger JIT compilation and because `form` field is trusted final, the JITed code will be using the uncustomized form for ever. Even after the customization thread plants the new form... This was possible before, but maybe the probability is greater after this change.

Thanks for the review, Peter.
The contract of `updateForm` clearly states that there are no guarantees provided about visibility:
    /**
     * Replace the old lambda form of this method handle with a new one.
     * The new one must be functionally equivalent to the old one.
     * Threads may continue running the old form indefinitely,
     * but it is likely that the new one will be preferred for new executions.
     * Use with discretion.
     */
It is used solely for performance reasons: install a faster LambdaForm variant and hope more users catch it.

Regarding `finally`, the intention of `updateInProgress` is to signal that there's a thread already preparing an updated form. Once it is done, the flag should be set back to `false` to allow more updates in the future. Indeed, there's a small window when another thread may succeed in acquiring the flag and performing the very same update, but (1) it's benign (it's still a performance optimization, so the more threads avoid redundant update the better);  and (2) there are fast-path checks in place (like `customized == mh` in `MH.customize()` or 'oldForm != newForm`) to detect such cases and don't waste resources.

Does it resolve your concerns?

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

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