[External] : Re: OM World and Lilliput planning

Tue Jun 18 21:49:05 UTC 2024

Now I need a reviewer.
https://bugs.openjdk.org/browse/JDK-8334496

I don't have to mention UseObjectMonitorTable and UseHeavyMonitors 
because they're going to be diagnostic flags.

thanks,
Coleen

On 6/18/24 11:59 AM, coleen.phillimore at oracle.com wrote:
>
> 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
>