[External] : Re: OM World and Lilliput planning

Tue Jun 18 15:59:37 UTC 2024

Hi Roman and Thomas, Thank you for your answers.

On 6/18/24 5:43 AM, Kennke, Roman wrote:
> The flag was never meant (by me) to be used by end-users. It’s been good to have it available for testing and development, and it’s served that purpose well. I’m happy to let it go.

Great.
>
> I’m a bit undecided about keeping the legacy code in 25. On the one hand, it feels safer to have the legacy as fallback for customers, in case any troubles arise. While LW locking and also recursive LW locking will be fairly well tested by then (at least by us), the OMWorld stuff will probably still have a trail of problems then. OTOH, keeping the legacy code around makes maintaining the locking code messy and more difficult, and legacy might even have started to bitrot by then (or not work at all, e.g. if Loom starts to rely on LW/OMWorld stuff).

If someone customer needs legacy locking going forward, it's because 
they have older code that suffers some large performance loss with 
lightweight locking, and more with the ObjectMonitor table (OM world).  
It might be severe enough to give them the switch to LM_LEGACY.  We 
haven't gotten that sort of feedback but it doesn't mean it won't 
exist.  For this reason, I think we should keep the Legacy code in JDK 
25, since it's an LTS and such a customer may only upgrade to LTS 
releases, and then remove it in 26.  Maintaining it won't be great in 
JDK 25, but the only backports we'd do from releases going forward would 
be bug fixes, so hopefully that code wouldn't bit rot badly there.  It 
does complicate the code, a lot, especially with the 
UseObjectMonitorTable option.

If Legacy is on, loom will pin synchronized code.

>
> I have similar feelings towards UseHeavyMonitors: on one hand it’s nice to be able to turn off the LW locking altogether, but experience shows that this has never really worked that well. For a long time, it would only turn off stack-locking in some paths (e.g. interpreter) but keep it in other (e.g. runtime), so it never really did what people thought it would do. And if we don’t carefully maintain and test this, it will bitrot, again. This is true whether or not we do it diagnostic or develop, though. I’d probably vote for diagnostic and then write some jtreg tests that verify that it does what it says, or remove it altogether.

It's a good idea to add some jtreg tests.  There are 4 of them now, and 
I ran tier1 with it turned on, and had one expected failure. You and 
Thomas voted diagnostic, so I'll make it that, so it's available in 
production if we need to do some testing with it.  Both diagnostic and 
develop flags can be removed anytime, so we could plan to remove it also 
in JDK 26.  That would be nice.
>
> I also kinda agree with Stefan Karlsson about making OMWorld turned on by default as soon as possible (maybe after giving it some short bake-time off-by-default, to make sure the old stuff still works as expected).

The rationale for turning OMWorld off by UseObjectMonitorTable option is 
because I ran some performance tests, and there are results that might 
not be acceptable, and we need time to sort them out or find some way of 
mitigating the regressions.  Our old friend, DaCapo xalan is a lot 
worse.  Also Dacapo spring-large, Renaissance-ScalaKmeans (really bad). 
Apart from these the results aren't significantly worse, but performance 
testing always eats up a lot of time, which is why we'd want OMWorld off 
to start.

Thanks,
Coleen

>
> Cheers,
> Roman
>
>
>> On Jun 18, 2024, at 11:26 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>> Hi Coleen,
>>
>> obsoleting the legacy mode feels a bit risky for an LTS. I am, of course, happy if the code can be removed.
>>
>> "and reintroduce (revert) the UseHeavyMonitors option as a develop flag, rather than a diagnostic flag"
>>
>> Why not diagnostic?
>>
>> Cheers, Thomas
>>
>>
>> On Mon, Jun 17, 2024 at 3:21 PM <coleen.phillimore at oracle.com> wrote:
>>
>> Hi Roman, Thomas and Aleksey,
>>
>> I filed an RFE for the first part of this.
>> https://bugs.openjdk.org/browse/JDK-8334299 and will be working on a
>> CSR.  I feel like we're backtracking a bit from what we did in JDK 22/23
>> but this seems better to me.  Comments?
>>
>> Thanks,
>> Coleen
>>
>> On 6/5/24 3:19 AM, Stefan Karlsson wrote:
>>> Hi Coleen,
>>>
>>> Thanks for moving the "OM World" towards completion. I have one
>>> comment below:
>>>
>>> On 2024-06-04 22:52, coleen.phillimore at oracle.com wrote:
>>>> Hi, This is what I wrote up after an internal discussion.  I am about
>>>> to file some RFEs/CSRs (or maybe will next week).  Let me know what
>>>> you think.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> What we call OM World is saving the ObjectMonitor in a
>>>> ConcurrentHashTable rather than in the markWord of the Java Object.
>>>> Lilliput absolutely requires this since for Lilliput the Klass
>>>> pointer is also in the markWord and to get to the Klass pointer for a
>>>> locked object, the code would have to go to the displaced header in
>>>> unboundedly racy situations.
>>>>
>>>> Without Lilliput, this is also helpful in that it frees up markWord
>>>> bits for concurrent GCs or Valhalla  to use. Because of this, and
>>>> because of the high level of testing this type of change requires,
>>>> we'd like to push this change to mainline ahead of the Lilliput work.
>>>>
>>>> OM World is built on top of Lightweight locking as Lightweight
>>>> locking is required (doesn't save the stack location in the markWord
>>>> as does Legacy locking).  To reduce the maintenance burden and
>>>> potential tricky interactions between new features and Legacy
>>>> locking, we'd like to deprecate Legacy locking in JDK 24.
>>>>
>>>> Deprecating Legacy locking then makes the flag LockingMode not make
>>>> any sense, as one of three enumerations will be missing. Also, to
>>>> introduce OM World on top of Lightweight locking, it would be good to
>>>> have that on a diagnostic flag in case of customer performance
>>>> issues.  It doesn't make sense to have a new locking mode for OM
>>>> World, since it shares 80% code with Lightweight locking.
>>>>
>>>> Therefore I (with input from Axel and Stefan) propose the following
>>>> for JDK 24:
>>>>
>>>> 1. Reintroduce the flag UseHeavyMonitors for LockingMode=LM_MONITOR
>>>> 2. Deprecate LockingMode=LM_LEGACY
>>>> 3. Deprecate the flag LockingMode.  It's a new flag, legacy code
>>>> won't miss it.
>>>> 4. When OM World is ready to integrate, introduce a new diagnostic
>>>> flag UseObjectMonitorTable
>>>>        - Start default off
>>>>        - Make it default on midway through JDK 24 if no problems.
>>> What is the benefit of starting with this turned off and then a few
>>> weeks later making it default? I think we'll get better functional
>>> test coverage if it is enabled by default. We had a very similar
>>> situation when Lightweight locking was turned off by default and many
>>> bugs weren't found until it was turned on by default.
>>>
>>> Thanks,
>>> StefanK
>>>
>>>> JDK 25:
>>>>
>>>> 1. Obsolete Legacy locking mode (removes the code - TBD)
>>>> 2. Obsolete LockingMode flag
>>>> 3. We can hold onto UseObjectMonitorTable for a while (off turns off
>>>> Lilliput UseCompactObjectHeaders).
>>>>
>
>
>
> Amazon Web Services Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
> Sitz: Berlin
> Ust-ID: DE 365 538 597