On Tue, 1 Dec 2020 15:55:36 GMT, Peter Levart <[email protected]> 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
