On Wed, 17 Aug 2022 15:34:01 GMT, Roman Kennke <rken...@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
