Hi Frank,
Thanks for the input—currently I'm contemplating whether it is not the most
efficient to perform the nulling out of the nextWaiter in the else-branch of
the update to firstWaiter and lastWaiter as such:
private void doSignal(ConditionNode first, boolean all) {
while (first != null) {
ConditionNode next = first.nextWaiter;
if ((firstWaiter = next) == null)
lastWaiter = null;
else
first.nextWaiter = null; // GC assistance
if ((first.getAndUnsetStatus(COND) & COND) != 0) {
enqueue(first);
if (!all)
break;
}
first = next;
}
}
The reason for this is that we're already branching, and the update to
`nextWaiter` is a plain write, which should be made visible earlier by the
update to the status succeeding it.
Updating nextWaiter after enqueue means that `first` has its COND cleared
*before*, which means that it should be eligible to be cleared by await:ers in
unlinkCancelledWaiters anyway.
I'll let Doug weigh in before I settle on anything.
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
________________________________
From: Frank Kretschmer <frank.kretsch...@gmx.net>
Sent: Monday, 19 February 2024 18:07
To: Viktor Klang <viktor.kl...@oracle.com>; Jaikiran Pai
<jai.forums2...@gmail.com>; Java Core Libs <core-libs-...@openjdk.java.net>
Subject: Re: [External] : Re: OpenJDK 17: Loitering
AbstractQueuedSynchronizer$ConditionNode instances (also on latest master
branch) [JDK-8325754]
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.kretsch...@gmx.net><mailto:frank.kretsch...@gmx.net>
Sent: Sunday, 18 February 2024 15:36
To: Jaikiran Pai <jai.forums2...@gmail.com><mailto:jai.forums2...@gmail.com>;
Viktor Klang <viktor.kl...@oracle.com><mailto:viktor.kl...@oracle.com>; Java
Core Libs
<core-libs-...@openjdk.java.net><mailto:core-libs-...@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.
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/ 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 “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
