RFR: 6507038: Memory Leak in JTree / BasicTreeUI [v4]
Alexey Ivanov
aivanov at openjdk.org
Tue Jan 30 16:17:07 UTC 2024
On Mon, 29 Jan 2024 06:18:50 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:
>> When using a TreeCellRenterer which creates new components in getTreeCellRendererComponent() in a JTree that is not visible, changes to the nodes cause a memory leak.
>> When a node is changed, the Method getNodeDimensions() is called to calculate the new dimensions for the node. In this method, getTreeCellRendererComponent() is called to obtain the renderer component (what else...) and [this component is added to rendererPane](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L3283-L3284). It is not removed from the rendererPane afterwards.
>> Only when the tree is painted, the paint() method does a removeAll on the rendererPane [in this code](https://github.com/openjdk/jdk/blob/36f4b34f1953af736706ec67192204727808bc6c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java#L1500)
>>
>> FIx is added to remove the components from rendererPane when the JTree UI is changed/uninstalled only when tree is not visible since they are already removed when tree is painted in paint() method..
>
> Prasanta Sadhukhan has updated the pull request incrementally with one additional commit since the last revision:
>
> Use removeAll and testcase modified
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTreeUI.java line 1:
> 1: /*
Update the copyright year in `BasicTreeUI`.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 51:
> 49: public class TreeCellRendererLeakTest {
> 50:
> 51: static int smCount = 0;
What does `sm-` prefix mean?
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 60:
> 58: private JTree jTree1;
> 59:
> 60: static CountDownLatch testDone;
Suggestion:
static final CountDownLatch testDone = new CountDownLatch(1);
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 62:
> 60: static CountDownLatch testDone;
> 61:
> 62: ArrayList<WeakReference<JLabel>> weakRefArrLabel = new ArrayList(50);
Suggestion:
final List<WeakReference<JLabel>> weakRefArrLabel = new ArrayList<>(50);
The type could be `List`. Use the diamond operator on the right side.
I suggest a simpler name: `labelList`, `labelRefList`, _`weakRefList`_.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 86:
> 84: }
> 85:
> 86: weakRefArrLabel.add(smCount++, new WeakReference<JLabel>(label));
Suggestion:
synchronized (weakRefArrLabel) {
weakRefArrLabel.add(new WeakReference<JLabel>(label));
}
You have to add items in a synchronized block; you read from the list in another thread. (Reading has be done inside a synchronized block too.)
Just add to the end of the list.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 136:
> 134: testDone = new CountDownLatch(1);
> 135: SwingUtilities.invokeAndWait(() -> {
> 136: TreeCellRendererLeakTest tf = new TreeCellRendererLeakTest();
Suggestion:
new TreeCellRendererLeakTest();
The variable is unused and can safely be removed. The side effects of calling the constructor won't disappear.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 164:
> 162: model.nodeChanged(n);
> 163: }
> 164: });
The anonymous class can be a lambda expression.
The code inside the `Runnable` can be reduced to
n.setUserObject("runcount " + currentCount);
model.nodeChanged(n);
This implies, you make `n` a field and initialise it in `initComponents`; or after calling `initComponents`.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 181:
> 179: public void runInfo() {
> 180: long time = System.currentTimeMillis();
> 181: long initialCnt = smCount;
`initialCnt` is unused in the current version.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 184:
> 182: while ((System.currentTimeMillis() - time) < (15 * 1000)) {
> 183: System.gc();
> 184: System.out.println("Live JLabels:" + smCount);
Access to `smCount` has to be synchronized between the two threads.
test/jdk/javax/swing/plaf/basic/BasicTreeUI/TreeCellRendererLeakTest.java line 204:
> 202: count++;
> 203: }
> 204: }
You have to iterate over `weakRefArrLabel` in a synchronized block. (No, declaring `weakRefArrLabel` as `volatile` is not enough.)
Suggestion:
synchronized (weakRefArrLabel) {
for (WeakReference<JLabel> ref : weakRefArrLabel) {
if (ref.get() == null) {
count++;
}
}
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/17458#pullrequestreview-1851588145
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471471871
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471467202
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471426426
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471432033
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471436951
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471438887
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471446967
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471468908
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471470295
PR Review Comment: https://git.openjdk.org/jdk/pull/17458#discussion_r1471458749
More information about the client-libs-dev
mailing list