On Tue, 1 Dec 2020 10:05:04 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> Concurrent updates may lead to redundant LambdaForms created and unnecessary >> class loading when those are compiled. >> >> Most notably, it severely affects MethodHandle customization: when a >> MethodHandle is called from multiple threads, every thread starts >> customization which takes enough time for other threads to join, but only >> one of those customizations will be picked. >> >> Coordination between threads requesting the updates and letting a single >> thread proceed avoids the aforementioned problem. Moreover, there's no need >> to wait until the update in-flight is over: all other threads (except the >> one performing the update) can just proceed with the invocation using the >> existing MH.form. >> >> Testing: >> - manually monitored the behavior on a stress test from >> [JDK-8252049](https://bugs.openjdk.java.net/browse/JDK-8252049) >> - tier1-4 > > 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; ------------- PR: https://git.openjdk.java.net/jdk/pull/1472
