On Mon, 5 Feb 2024 16:38:59 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Good catch. Your solution might be correct but I think `!contains(e)` is
>> redundant since that is how intern starts out.
>>
>>
>> static <T> T intern(ReferencedKeyMap<T, ReferenceKey<T>> setMap, T key,
>> UnaryOperator<T> interner) {
>> T value = existingKey(setMap, key);
>> if (value != null) {
>> return value;
>> }
>> key = interner.apply(key);
>> return internKey(setMap, key);
>> }
>>
>>
>> Agree? So changing to `return intern(e) == e;` should be sufficient.
>>
>> The other aspect of this is that `internKey` uses `putIfAbsent` which should
>> prevent the race (assuming `ConcurrentHashMap`).
>>
>>
>> /**
>> * Attempt to add key to map.
>> *
>> * @param setMap {@link ReferencedKeyMap} where interning takes place
>> * @param key key to add
>> *
>> * @param <T> type of key
>> *
>> * @return the old key instance if found otherwise the new key instance
>> */
>> private static <T> T internKey(ReferencedKeyMap<T, ReferenceKey<T>>
>> setMap, T key) {
>> ReferenceKey<T> entryKey = setMap.entryKey(key);
>> T interned;
>> do {
>> setMap.removeStaleReferences();
>> ReferenceKey<T> existing = setMap.map.putIfAbsent(entryKey,
>> entryKey);
>> if (existing == null) {
>> return key;
>> } else {
>> // If {@code putIfAbsent} returns non-null then was actually
>> a
>> // {@code replace} and older key was used. In that case the
>> new
>> // key was not used and the reference marked stale.
>> interned = existing.get();
>> if (interned != null) {
>> entryKey.unused();
>> }
>> }
>> } while (interned == null);
>> return interned;
>> }
>>
>>
>> Agree? Anyone else want to pipe in?
>
> ok, `intern(e) == e` is sufficient.
Created https://bugs.openjdk.org/browse/JDK-8325255
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1478583967
