RFR: 8291555: Replace stack-locking with fast-locking

[Vladimir Kozlov]

On Wed, 17 Aug 2022 15:34:01 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>>> Strictly speaking, I believe the conditions check for the (weaker) balanced property, but not for the (stronger) structured property.
>> 
>> I know but the text says:
>> - "every exit on a given monitor matches a preceding entry on that monitor."
>> - "implementations of the Java Virtual Machine are permitted but not required to enforce both of the following two rules guaranteeing structured locking"
>> 
>> I read this as if the rules do not guarantee structured locking the rules are not correct.
>> The VM is allowed to enforce it.
>> But thats just my take on it.
>> 
>> EDIT:
>> Maybe I'm reading to much into it.
>> Lock A,B then unlock A,B maybe is considered structured locking?
>> 
>> But then again what if:
>> 
>> 
>> void foo_lock() {
>>   monitorenter(A);
>>   monitorenter(B);
>>   // If VM abruptly returns here
>>   // VM can unlock them in reverse order first B and then A ?
>>   monitorexit(A);
>>   monitorexit(B);
>> }
>
>> > Strictly speaking, I believe the conditions check for the (weaker) balanced property, but not for the (stronger) structured property.
>> 
>> I know but the text says:
>> 
>>     * "every exit on a given monitor matches a preceding entry on that monitor."
>> 
>>     * "implementations of the Java Virtual Machine are permitted but not required to enforce both of the following two rules guaranteeing structured locking"
>> 
>> 
>> I read this as if the rules do not guarantee structured locking the rules are not correct. The VM is allowed to enforce it. But thats just my take on it.
>> 
>> EDIT: Maybe I'm reading to much into it. Lock A,B then unlock A,B maybe is considered structured locking?
>> 
>> But then again what if:
>> 
>> ```
>> void foo_lock() {
>>   monitorenter(A);
>>   monitorenter(B);
>>   // If VM abruptly returns here
>>   // VM can unlock them in reverse order first B and then A ?
>>   monitorexit(A);
>>   monitorexit(B);
>> }
>> ```
> 
> Do you think there would be any chance to clarify the spec there? Or even outright disallow unstructured/not-properly-nested locking altogether (and maybe allow the verifier to check it)? That would certainly be the right thing to do. And, afaict, it would do no harm because no compiler of any language would ever emit unstructured locking anyway - because if it did, the resulting code would crawl interpreted-only).

We need to understand performance effects of these changes. I don't see data here or new JMH benchmarks which can show data. @rkennke can you show data you have?  And, please, update RFE description with what you have in PR description.

@ericcaspole do we have JMH benchmarks to test performance for different lock scenarios?
I see few tests in `test/micro` which use `synchronized`. Are they enough? Or we need more?
Do we have internal benchmarks we could use for such testing?

I would prefer to have "opt-in" but looking on scope of changes it may introduce more issues.
Without "opt-in" I want performance comparison of VMs with different implementation instead of using `UseHeavyMonitors` to make judgement about this implementation.
`UseHeavyMonitors` (product flag) should be tested separately to make sure when it is used as fallback mechanism by customers they would not get significant performance penalty.

I agree with @tstuefe that we should test this PR a lot (all tiers on all supported platforms) including performance testing before integration. In addition we need full testing of this implementation with `UseHeavyMonitors` ON.

And I should repeat that integration happens when changes are ready (no issues). We should not rush for particular JDK release.

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

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