Skip to content

8355369: Remove setAccessible usage for setting final fields in java.util.concurrent #24821

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 2 commits into from
Closed

8355369: Remove setAccessible usage for setting final fields in java.util.concurrent #24821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6e7ac0
Update ConcurrentSkipListSet, CopyOnWriteArrayList, and AtomicReferen…
viktorklang-ora Apr 23, 2025
39dba5d
Adding a releaseFence() to ConcurrentskipListSet.clone()
viktorklang-ora Apr 23, 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
19 changes: 11 additions & 8 deletions src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@

package java.util.concurrent;

import java.lang.reflect.Field;
import jdk.internal.misc.Unsafe;

import java.lang.invoke.VarHandle;
import java.util.AbstractSet;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -176,6 +178,8 @@ public ConcurrentSkipListSet<E> clone() {
ConcurrentSkipListSet<E> clone =
(ConcurrentSkipListSet<E>) super.clone();
clone.setMap(new ConcurrentSkipListMap<E,Object>(m));
// Needed to ensure safe publication of setMap()
VarHandle.releaseFence();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DougLea This might be interesting to you. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could just change it to return new ConcurrentSkipListSet<>(m). COWAL could be changed to return a new object too.

Copy link
Member

Choose a reason for hiding this comment

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

The set only has a map field, and AbstractSet does not define any additional field. The map should be fine too - two fields in AbstractMap are cleared when cloning happens, so recreating a map from a constructor should have the same effect. (Note a significant field head is not cleared upon clone, but seems immediately replaced later in buildFromSorted). Both should still be fine with this new return values.

Copy link
Contributor

Choose a reason for hiding this comment

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

As others have mentioned, clone() needs to be re-checked wrt issuing final field fences. But can be conservatively done so here anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DougLea Yeah. I'd be surprised if clone()-invocations were specially tracked to get a trailing release fence inserted before handing out the instance.

return clone;
} catch (CloneNotSupportedException e) {
throw new InternalError();
Expand Down Expand Up @@ -527,12 +531,11 @@ public Spliterator<E> spliterator() {

/** Initializes map field; for use in clone. */
private void setMap(ConcurrentNavigableMap<E,Object> map) {
try {
Field mapField = ConcurrentSkipListSet.class.getDeclaredField("m");
mapField.setAccessible(true);
mapField.set(this, map);
} catch (IllegalAccessException | NoSuchFieldException e) {
throw new Error(e);
}
final Unsafe U = Unsafe.getUnsafe();
U.putReference(
this,
U.objectFieldOffset(ConcurrentSkipListSet.class, "m"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to compute the offset once and for all and put it in a static final long field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I originally did that, but the following patch is the "smallest change".
Given that the "original code" obtained the Field instance each call, this is still likely a performance improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

This code has always used getDeclaredField each time so if anyone had run into a performance issue then we should have heard by now. So I think keeping the changes simple and just moving to Unsafe is okay for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine. The main effect of this change is to nearly revert to the pre-VarHandle version of this code.

map
);
}
}
15 changes: 7 additions & 8 deletions src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
package java.util.concurrent;

import java.lang.invoke.VarHandle;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
Expand All @@ -57,6 +56,7 @@
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
import jdk.internal.access.SharedSecrets;
import jdk.internal.misc.Unsafe;
import jdk.internal.util.ArraysSupport;

/**
Expand Down Expand Up @@ -2095,12 +2095,11 @@ public List<E> reversed() {

/** Initializes the lock; for use when deserializing or cloning. */
private void resetLock() {
try {
Field lockField = CopyOnWriteArrayList.class.getDeclaredField("lock");
lockField.setAccessible(true);
lockField.set(this, new Object());
} catch (IllegalAccessException | NoSuchFieldException e) {
throw new Error(e);
}
final Unsafe U = Unsafe.getUnsafe();
U.putReference(
this,
U.objectFieldOffset(CopyOnWriteArrayList.class, "lock"),
new Object()
);
}
}
17 changes: 8 additions & 9 deletions src/java.base/share/classes/java/util/concurrent/atomic/AtomicReferenceArray.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@

package java.util.concurrent.atomic;

import jdk.internal.misc.Unsafe;

import java.lang.invoke.MethodHandles;
import java.lang.invoke.VarHandle;
import java.lang.reflect.Array;
import java.lang.reflect.Field;
import java.util.Arrays;
import java.util.function.BinaryOperator;
import java.util.function.UnaryOperator;
Expand Down Expand Up @@ -330,14 +331,13 @@ private void readObject(java.io.ObjectInputStream s)
throw new java.io.InvalidObjectException("Not array type");
if (a.getClass() != Object[].class)
a = Arrays.copyOf((Object[])a, Array.getLength(a), Object[].class);
try {

Field arrayField = AtomicReferenceArray.class.getDeclaredField("array");
arrayField.setAccessible(true);
arrayField.set(this, a);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new Error(e);
}
final Unsafe U = Unsafe.getUnsafe();
U.putReference(
this,
U.objectFieldOffset(AtomicReferenceArray.class, "array"),
a
);
}

// jdk9
Expand Down Expand Up @@ -523,5 +523,4 @@ public final boolean weakCompareAndSetAcquire(int i, E expectedValue, E newValue
public final boolean weakCompareAndSetRelease(int i, E expectedValue, E newValue) {
return AA.weakCompareAndSetRelease(array, i, expectedValue, newValue);
}

}