Skip to content

8362289: [macOS] Remove finalize method in JRSUIControls.java #26331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

8362289: [macOS] Remove finalize method in JRSUIControls.java #26331

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f72b109
8362289
prrace Jul 15, 2025
130fdc5
8362289
prrace Jul 15, 2025
2c56173
8362289
prrace Jul 16, 2025
21ad3a1
Merge branch 'master' into jrsuicontrols
prrace Jul 16, 2025
3b4556c
JRSUIControl.java
prrace Jul 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions src/java.desktop/macosx/classes/apple/laf/JRSUIControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

import java.nio.*;
import java.util.*;
import sun.java2d.Disposer;
import sun.java2d.DisposerRecord;

import apple.laf.JRSUIConstants.*;

Expand Down Expand Up @@ -91,7 +93,8 @@ private static ThreadLocalByteBuffer getThreadLocalBuffer() {

private final HashMap<Key, DoubleValue> nativeMap;
private final HashMap<Key, DoubleValue> changes;
private long cfDictionaryPtr;
private final long cfDictionaryPtr;
private final Object disposerReferent = new Object();

private long priorEncodedProperties;
private long currentEncodedProperties;
Expand All @@ -101,6 +104,7 @@ public JRSUIControl(final boolean flipped){
this.flipped = flipped;
cfDictionaryPtr = getCFDictionary(flipped);
if (cfDictionaryPtr == 0) throw new RuntimeException("Unable to create native representation");
Disposer.addRecord(disposerReferent, new JRSUIControlDisposerRecord(cfDictionaryPtr));
nativeMap = new HashMap<Key, DoubleValue>();
changes = new HashMap<Key, DoubleValue>();
}
Expand All @@ -109,17 +113,25 @@ public JRSUIControl(final boolean flipped){
flipped = other.flipped;
cfDictionaryPtr = getCFDictionary(flipped);
if (cfDictionaryPtr == 0) throw new RuntimeException("Unable to create native representation");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a side topic to discuss: In the past I tried to look into this pattern and figure out is it possible for the GC to start cleaning the object after we allocate the native resource but before we register it with Disposer.addRecord. In such a case, the cleanup might never be executed.

Perhaps we should consider adding a reachabilityFence at the end of Disposer.addRecord, for both the DisposerRecord and the referent, to ensure they remain reachable. Similarly how it was done for cleaner:
https://hg.openjdk.org/jdk9/jdk9/jdk/rev/1f33e517236e
Or may be we can just use cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is even a bug for Cleaner that the reachabilityFence should be moved to the end of register:
https://bugs.openjdk.org/browse/JDK-8291867
If there are no objectiosn I'll add it to the Disposer.addRecord in a separate bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be OK, meaning probably harmless. But I don't know if this case actually requires it.
Where I've seen a problem like this a Java object is used at an earlier point in the method and not again and so becomes eligible to be freed, but some native resource it held is needed later but is already freed along with its no longer references holder.

Here, if this object (the JRSUIControl) becomes unreachable during construction, and so the referent and disposer are also unreachable outside this object, they are still going to be live until we reach code in the constructor which uses them and since the referent and the disposer are passed into addRecord they'll be live there until stored.

But these cases are subtle, maybe I'm missing something. Can you expand on the scenario ?
Or maybe there's some other patterns where we use Disposer that it is needed ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand why reachabilityFence is used here. That code first registers the object in the cleanup queue and then continues executing the rest of the constructor. As far as I understand, the object could be collected between the registration and the end of the constructor so the cleaner will be called on a partially constructed object.

If that assumption is correct then in the change above the code:
nativeMap = new HashMap<Key, DoubleValue>();
changes = new HashMap<Key, DoubleValue>();
might never be executed and the native disposer will be called before, not an issue in this particular case but good to know.
Or I missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's right. At first I thought you were referring to the constructor of the thing that wants to use the cleaner, but you mean the constructor of the Cleanable itself may be still under construction when the referent is collected and installing the reachabilityFence at the end of the constructor prevents that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to compare how Cleaner and Disposer are used. I found that in most cases Cleaner uses this as the object to track. But Disposer often uses a separate object like disposerReferent or some kind of anchor. This patch is one example. In some places we even replaced this with a different object. You can see that in the following RFR:
https://bugs.openjdk.org/browse/JDK-8030099
https://mail.openjdk.org/pipermail/swing-dev/2015-October/005053.html

I think I found the reason: https://bugs.openjdk.org/browse/JDK-8071507

In older versions of the JDK the Disposer had to use phantom references. Phantom references allow native cleanup to happen after the object is 100% no longer reachable. But this had a problem, the object stayed in memory until the reference was added to the queue and the code called clear.

To avoid keeping large AWT or Java2D objects in memory for too long the code used small helper objects as referents instead of using the real object.

Now the problem described in JDK-8071507 is fixed. So maybe we no longer need to use small disposer referents. And you can check that by the test from the description of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is possible, but I'd prefer to keep this pattern. This way It is crystal clear that nothing to do with reference queues etc are delaying the GC from immediately reclaiming the memory.
Honestly, it probably matters not so much for JRSUIControls, and even less so for CGraphicsEnvironment, but may matter for some other cases.

A later follow-up fix could go change a whole bunch of these cases at once and we'd then have a good reference (pun intended) as to whether any issues showed up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

Disposer.addRecord(disposerReferent, new JRSUIControlDisposerRecord(cfDictionaryPtr));
nativeMap = new HashMap<Key, DoubleValue>();
changes = new HashMap<Key, DoubleValue>(other.nativeMap);
changes.putAll(other.changes);
}

@Override
@SuppressWarnings("removal")
protected synchronized void finalize() throws Throwable {
if (cfDictionaryPtr == 0) return;
disposeCFDictionary(cfDictionaryPtr);
cfDictionaryPtr = 0;
private static class JRSUIControlDisposerRecord implements DisposerRecord {

private final long cfDictionaryPtr;
JRSUIControlDisposerRecord(long ptr) {
cfDictionaryPtr = ptr;
}

public void dispose() {
try {
disposeCFDictionary(cfDictionaryPtr);
} catch (Throwable t) {
}
}
}


Expand Down