RFR: 8257189: Handle concurrent updates of MH.form better

[Peter Levart]

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

>> src/java.base/share/classes/java/lang/invoke/MethodHandle.java line 1783:
>> 
>>> 1781:             } finally {
>>> 1782:                 updateInProgress = false;
>>> 1783:             }
>> 
>> Line 1782. I understand that in case the try block fails to update `form` field, resetting `updateInProgress` is desirable in order for next caller to attempt to update the form next time, but in case (the majority case) the try block successfully updates the `form` field, resetting the `updateInProgress` back to `false` opens a theoretical window (very improbable) for a concurrent thread to re-execute the updater function at least. Perhaps you could write `catch (Throwable e)` instead of `finally` and only in case of failure, reset the `updateInProgress` field and re-throw the exception. WDYT?
>> 
>> It is also very strange that the code is updating a final `form` field using Unsafe. Is this only to ensure that reads of that field scattered around in code are accompanied with a suitable read fence if needed (on some architectures)? How does this work considering (I may be wrong, but I think I heard) that VM trusts final fields in java.lang.invoke package.
>
> 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.

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

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