[External] : Re: OpenJDK 17: Loitering AbstractQueuedSynchronizer$ConditionNode instances (also on latest master branch) [JDK-8325754]
Frank Kretschmer
frank.kretschmer at gmx.net
Mon Feb 19 17:07:56 UTC 2024
Hi Viktor,
may I add one option to your evaluation?
@@ -1506,14 +1506,15 @@ public ConditionObject() { }
private void doSignal(ConditionNode first, boolean all) {
while (first != null) {
ConditionNode next = first.nextWaiter;
if ((firstWaiter = next) == null)
lastWaiter = null;
if ((first.getAndUnsetStatus(COND) & COND) != 0) {
enqueue(first);
+ first.nextWaiter = null; // GC-friendly
if (!all)
break;
}
first = next;
}
}
(AbstractQueuedSynchronizer line numbers as in gitlab current master)
This variant takes care about race conditions on cancellation
(unlinkCancelledWaiters() needs 'nextWaiter'), as thanks to
"getAndUnsetStatus(COND) & COND) != 0" only alternatively/once executed.
So this option is definitively better / more robust than my first one 🙂
Best regards
Frank
Am 19.02.2024 um 12:41 schrieb Viktor Klang:
> Hi Frank,
>
> We'll see what the option are. 🙂
>
> Cheers,
> √
>
> *
> *
> *Viktor Klang*
> Software Architect, Java Platform Group
> Oracle
> ------------------------------------------------------------------------
> *From:* Frank Kretschmer <frank.kretschmer at gmx.net>
> *Sent:* Sunday, 18 February 2024 15:36
> *To:* Jaikiran Pai <jai.forums2013 at gmail.com>; Viktor Klang
> <viktor.klang at oracle.com>; Java Core Libs <core-libs-dev at openjdk.java.net>
> *Subject:* [External] : Re: OpenJDK 17: Loitering
> AbstractQueuedSynchronizer$ConditionNode instances (also on latest
> master branch) [JDK-8325754]
>
> Hello Jaikiran, hello Viktor,
>
> in the meantime, I've seen that the JBS issue has been assigned to
> Viktor Klang. @Viktor: I totally agree with your comment that the
> proposed solution may not be the best possible option, and that
> further explorations were required.
>
> My intention to propose unlinking ConditionNodes by null'ing their
> ‘nextWaiter’ reference was just to verify that the chain of
> ‘nextWaiter’ references leads to the observed garbage collection
> behavior, and that the GC is able to collect the nodes during minor /
> young collections if the references are cleaned in time.
>
> I checked also a few other variants (null'ing the ‘nextWaiter’
> reference at the end of all await...() methods in ConditionObject, or
> in/just before enqueue()), but at the end of the day, I felt that
> null'ing it in doSignal() explains what I want to show the easiest. On
> the other hand, the other options could be better in order to avoid
> race conditions with canceled nodes.
>
> For sure there are many other options that I am not aware of, so
> please take my proposal just as an example.
>
> Looking forward to your explorations.
>
> Best regards
>
> Frank
>
>
> Am 14.02.2024 um 07:43 schrieb Jaikiran Pai:
>>
>> Hello Frank,
>>
>> I see that a JBS issue has been created for this same issue
>> https://bugs.openjdk.org/browse/JDK-8325754
>> <https://bugs.openjdk.org/browse/JDK-8325754>.
>>
>> I don't have enough knowledge of this area and haven't reviewed this
>> part of the code in detail to see if there are any obvious issues
>> with what you are proposing as a solution. Since there's now a JBS
>> issue created for this and you seem to have done enough investigation
>> and work on this one already, would you be interested in creating a
>> pull request against the https://github.com/openjdk/jdk
>> <https://urldefense.com/v3/__https://github.com/openjdk/jdk__;!!ACWV5N9M2RV99hQ!NtWokgpYjFyT0Gdq0NiTif6NvtYcNz39rzE7qzmJsQi5X_KwWSMhmV16WfkPx_5ByfNe4J-pgT8gyqCLKofbXZ9rkczUDg$>
>> repo with this proposed change? (you'll have to sign a OCA). This
>> guide https://openjdk.org/guide/ <https://openjdk.org/guide/> should
>> help you get started. It can then go through the usual reviews that a
>> bug fix/enhancement goes through.
>>
>> -Jaikiran
>>
>> On 11/02/24 7:27 pm, Frank Kretschmer wrote:
>>>
>>> Hello Core-Libs-Dev team,
>>>
>>> may I ask you about your opinion about a tiny one-liner change in
>>> AbstractQueuedSynchronizer, just as a suggestion how to make
>>> ConditionObjects / Nodes even more garbage collector friendly?
>>>
>>> Checked out
>>> https://github.com/openjdk/jdk/blob/jdk-17%2B35/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
>>> <https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/jdk-17*2B35/src/java.base/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java__;JQ!!ACWV5N9M2RV99hQ!NtWokgpYjFyT0Gdq0NiTif6NvtYcNz39rzE7qzmJsQi5X_KwWSMhmV16WfkPx_5ByfNe4J-pgT8gyqCLKofbXZ8EWBUt0w$>
>>> (the same on master branch with different line numbers near to line
>>> 1506):
>>>
>>> @@ -1431,40 +1431,41 @@ public abstract class AbstractQueuedSynchronizer
>>> public class ConditionObject implements Condition,
>>> java.io.Serializable {
>>> // ...
>>> private void doSignal(ConditionNode first, boolean all) {
>>> while (first != null) {
>>> ConditionNode next = first.nextWaiter;
>>> + first.nextWaiter = null; // GC-friendly: avoid
>>> chains of dead ConditionNodes
>>> if ((firstWaiter = next) == null)
>>> lastWaiter = null;
>>> if ((first.getAndUnsetStatus(COND) & COND) != 0) {
>>> enqueue(first);
>>> // ...
>>>
>>> By setting the nextWaiter to null of the first condition node, which
>>> is transferred from the condition queue to the sync queue in this
>>> method, long chains of ConditionNode instances can be avoided.
>>> Though a single ConditionNode is small, these chains of
>>> ConditionNodes become very huge on the heap (I've seen more than 1GB
>>> on an application server over time) if at least one node was
>>> promoted to the old generation for any reason. They survive minor
>>> collections and are cleaned up only on mixed / full collections, and
>>> thus put unnecessary pressure on G1 garbage collector.
>>>
>>> The same change could also be applied to
>>> 'AbstractQueuedLongSynchronizer'.
>>>
>>> I know premature optimization is the root of all evil, on the other
>>> hand I could image that many applications benefit from GC-friendly
>>> ConditionObjects, since they are frequently used in various classes
>>> like PriorityBlockingQueue / LinkedBlockingDeque /
>>> LinkedBlockingQueue, the latter one as default work queue for
>>> executor services like fixed thread pools for processing
>>> asynchronous tasks.
>>>
>>> Thank you all for your time and help!
>>>
>>> Best regards
>>> Frank
>>>
>>> Am 08.02.2024 um 12:15 schrieb Frank Kretschmer:
>>>> Hello Thomas, hello Core-Libs-Dev,
>>>>
>>>> thank you for cc'ing my email. In deed my idea/suggestion is to modify
>>>> the AbstractQueuedSynchronizer$ConditionNode handling in such a way
>>>> that
>>>> it gets unlinked from the chain of condition nodes if it is not needed
>>>> any more (it might be the "nextWaiter" node), in order to be more
>>>> GC-friendly.
>>>>
>>>> @core-libs-dev: I've just attached the “G1LoiteringConditionNodes”
>>>> demo
>>>> class and "gc.log" again so that you can have a look if you like.
>>>>
>>>> Best regards
>>>>
>>>> Frank
>>>>
>>>>
>>>> Am 08.02.2024 um 11:04 schrieb Thomas Schatzl:
>>>>> Hi,
>>>>>
>>>>> since this looks like a suggestion for a change to the libraries
>>>>> similar to the mentioned JDK-6805775, and not actually GC, cc'ing the
>>>>> core-libs-dev mailing list.
>>>>>
>>>>> Hth,
>>>>> Thomas
>>>>>
>>>>> On 07.02.24 15:20, Frank Kretschmer wrote:
>>>>>> Hi Java GC-experts,
>>>>>>
>>>>>> I'm facing an interesting G1 garbage collector observation in
>>>>>> OpenJDK
>>>>>> 17.0.9+9, which I would like to share with you.
>>>>>>
>>>>>> My application runs many asynchronous tasks in a fixed thread pool,
>>>>>> utilizing its standard LinkedBlockingQueue. Usually, it generates
>>>>>> just a
>>>>>> little garbage, but from time to time, I observed that the survivor
>>>>>> spaces grow unexpectedly, and minor collection times increase.
>>>>>>
>>>>>> This being the case, many
>>>>>> java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode
>>>>>> instances can be found on the heap. In fact, the whole heap (rank
>>>>>> 1 as
>>>>>> shown in jmap) was filled up with ConditionNode instances after a
>>>>>> while.
>>>>>>
>>>>>> After some tests, I figured out that G1 seems to be able to collect
>>>>>> “dead” ConditionNode instances during minor collections only if no
>>>>>> formerly alive ConditionNode instances were promoted to the old
>>>>>> generation and died there.
>>>>>>
>>>>>> To illustrate that, I've attached a “G1LoiteringConditionNodes”
>>>>>> class
>>>>>> that can be run for demo purposes, e.g. under Linux with OpenJDK
>>>>>> 17.0.9+9 (VM options see comments within the class), and its gc-log
>>>>>> output. It shows that during the first two minutes, everything is
>>>>>> fine,
>>>>>> but after a promotion to the old generation, survivors grow and
>>>>>> minor
>>>>>> pause time increase from 3 to 10ms.
>>>>>>
>>>>>> For me, it looks like an issue similar to
>>>>>> https://bugs.openjdk.org/browse/JDK-6805775
>>>>>> <https://bugs.openjdk.org/browse/JDK-6805775>
>>>>>> “LinkedBlockingQueue Nodes
>>>>>> should unlink themselves before becoming garbage”, which was
>>>>>> fixed in
>>>>>> OpenJDK 7.
>>>>>>
>>>>>> What’s your opinion about that? Wouldn’t it be worth to enable G1 to
>>>>>> collect those AbstractQueuedSynchronizer$ConditionNode instances
>>>>>> during
>>>>>> minor collections, as it is done for LinkedBlockingQueue Nodes?
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Frank
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240219/226f78e8/attachment-0001.htm>
More information about the core-libs-dev
mailing list