Take the global monitors off the marshalling path

orbmain/src/main/java/com/sun/corba/ee/impl/io/ObjectStreamClass.java

@@ -22,6 +22,7 @@
 package com.sun.corba.ee.impl.io;
 
 import com.sun.corba.ee.impl.misc.ClassInfoCache;
+import com.sun.corba.ee.impl.misc.ConcurrentSoftCache;
 import com.sun.corba.ee.impl.util.RepositoryId;
 import com.sun.corba.ee.spi.trace.TraceValueHandler;
 
@@ -46,8 +47,8 @@
 import java.util.Comparator;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentMap;
 
-import org.glassfish.pfl.basic.concurrent.SoftCache;
 import org.glassfish.pfl.basic.reflection.Bridge;
 import org.omg.CORBA.ValueMember;
 
@@ -99,13 +100,39 @@ static final ObjectStreamClass lookup(Class<?> cl) {
      */
     static ObjectStreamClass lookupInternal(Class<?> cl)
     {
+        /* A descriptor that is present and fully initialized can be returned
+         * without any lock at all, and in a running system that is very nearly
+         * every lookup: descriptors are created once per class and then read
+         * for the life of the process.
+         *
+         * The lock below is a single process wide monitor which every
+         * marshalled and unmarshalled object had to pass through, so removing
+         * it from the steady state path is the point of this fast path. It
+         * also has to be a fast path rather than a rewrite of what follows:
+         * the comment on init() records that moving initialization out of this
+         * monitor was tried and reverted because it deadlocks (bug 5104239),
+         * so the slow path is deliberately left exactly as it was.
+         *
+         * Correctness rests on two things. 'initialized' is volatile and is
+         * assigned last in init(), so seeing it true means every field written
+         * during initialization is visible. And a descriptor is published into
+         * the cache by the constructor, before init() runs, precisely so that
+         * recursive lookups find it - which is why it is not enough for the
+         * entry to exist, it must also report itself initialized.
+         */
+        ObjectStreamClass cached = descriptorFor.get(cl);
+        if (cached != null && cached.initialized) {
+            return cached;
+        }
+
         /* Synchronize on the hashtable so no two threads will do
          * this at the same time.
          */
         ObjectStreamClass desc = null;
         synchronized (descriptorFor) {
+            descriptorFor.purge();
             /* Find the matching descriptor if it already known */
-            desc = (ObjectStreamClass)descriptorFor.get( cl ) ;
+            desc = descriptorFor.get( cl ) ;
             if (desc == null) {
                 /* Check if it's serializable */
                 ClassInfoCache.ClassInfo cinfo = ClassInfoCache.get( cl ) ;
@@ -641,7 +668,15 @@ public final String getRMIIIOPOptionalDataRepId() {
         superclass = null;
     }
 
-    public static final synchronized ObjectStreamField[] translateFields(
+    /**
+     * @param fields the fields to translate
+     * @return the translated fields
+     */
+    // Not synchronized: PersistentFieldsValue.translateFields allocates a new
+    // array and reads only its argument, so the monitor this used to take -
+    // on the ObjectStreamClass class object, shared with every other static
+    // synchronized member - guarded nothing.
+    public static final ObjectStreamField[] translateFields(
             java.io.ObjectStreamField fields[]) {
         return PersistentFieldsValue.translateFields(fields);
     }
@@ -1194,11 +1229,18 @@ static String getSignature(Constructor<?> cons) {
         return sb.toString();
     }
 
-    /*
-     * Cache of Class -> ClassDescriptor Mappings.
+    /**
+     * Cache of Class to ObjectStreamClass mappings.
+     *
+     * <p>This used to be {@code org.glassfish.pfl.basic.concurrent.SoftCache},
+     * a bare HashMap that was safe only because {@link #lookupInternal} took a
+     * process wide monitor around every access. Reads have to be lock free for
+     * that method's fast path to exist. The values stay soft: an
+     * ObjectStreamClass holds its Class, so a strong map would form a key to
+     * value to key cycle pinning the application class loader.
      */
-    static private final SoftCache<Class<?>,ObjectStreamClass> descriptorFor =
-        new SoftCache<Class<?>,ObjectStreamClass>() ;
+    private static final ConcurrentSoftCache<Class<?>, ObjectStreamClass> descriptorFor =
+        new ConcurrentSoftCache<>();
 
     /*
      * The name of this descriptor
@@ -1258,7 +1300,10 @@ static String getSignature(Constructor<?> cons) {
      * try to fix bug 4373844.  Working to move to
      * reusing java.io.ObjectStreamClass for JDK 1.5.
      */
-    private boolean initialized = false;
+    // Read without holding any lock by lookupInternal's fast path, and
+    // assigned last by init(), so it doubles as the publication fence for
+    // every other field this descriptor computes.
+    private volatile boolean initialized = false;
 
     /* Internal lock object. */
     private final Object lock = new Object();

orbmain/src/main/java/com/sun/corba/ee/impl/misc/ConcurrentSoftCache.java

@@ -0,0 +1,105 @@
+/*
+ * Copyright (c) 2026 Contributors to the Eclipse Foundation.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0, or the Eclipse Distribution License
+ * v. 1.0 which is available at
+ * http://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the Eclipse
+ * Public License v. 2.0 are satisfied: GNU General Public License v2.0
+ * w/Classpath exception which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause OR GPL-2.0 WITH
+ * Classpath-exception-2.0
+ */
+
+package com.sun.corba.ee.impl.misc;
+
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.SoftReference;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+/**
+ * A concurrent cache whose values are held softly.
+ *
+ * <p>It exists to replace two uses of
+ * {@code org.glassfish.pfl.basic.concurrent.SoftCache}, which despite its
+ * package name is a bare {@code HashMap} with no synchronization of its own.
+ * One of those uses was safe only because every caller happened to hold an
+ * external monitor; the other was read with no lock at all while other
+ * threads wrote it, which is a data race on a plain HashMap - and since that
+ * class also mutates its map inside {@code get}, even two concurrent readers
+ * were enough to corrupt it.
+ *
+ * <p>Values are soft rather than strong because both caches map to, or from,
+ * {@code Class} objects. A strong reference from a process wide static map to
+ * an application class keeps its class loader alive forever, which in an
+ * application server is a redeployment leak.
+ *
+ * <p>Reads take no lock. Cleared entries are evicted by {@link #purge()},
+ * which callers are expected to invoke from whatever slow path they already
+ * have, rather than on every read.
+ *
+ * @param <K> key type
+ * @param <V> value type
+ */
+public final class ConcurrentSoftCache<K, V> {
+
+    private final ConcurrentMap<K, Entry<K, V>> map = new ConcurrentHashMap<>();
+    private final ReferenceQueue<V> cleared = new ReferenceQueue<>();
+
+    /** A soft reference that remembers its key, so a cleared one can be evicted. */
+    private static final class Entry<K, V> extends SoftReference<V> {
+
+        private final K key;
+
+        Entry(K key, V value, ReferenceQueue<V> queue) {
+            super(value, queue);
+            this.key = key;
+        }
+    }
+
+    /**
+     * @param key the key to look up
+     * @return the value, or null if absent or already collected
+     */
+    public V get(K key) {
+        Entry<K, V> entry = map.get(key);
+        return entry == null ? null : entry.get();
+    }
+
+    /**
+     * @param key the key to store under
+     * @param value the value to hold softly
+     */
+    public void put(K key, V value) {
+        map.put(key, new Entry<>(key, value, cleared));
+    }
+
+    /**
+     * Drops the entries whose value has been collected.
+     */
+    public void purge() {
+        for (Reference<? extends V> ref; (ref = cleared.poll()) != null; ) {
+            @SuppressWarnings("unchecked")
+            Entry<K, V> entry = (Entry<K, V>) ref;
+            // Two argument remove, so an entry that a later put reinstated
+            // under the same key is never evicted by this one's death.
+            map.remove(entry.key, entry);
+        }
+    }
+
+    /**
+     * @return the number of entries, including any whose value has been
+     *         collected but not yet purged
+     */
+    public int size() {
+        return map.size();
+    }
+}

orbmain/src/main/java/com/sun/corba/ee/impl/util/RepositoryId.java

@@ -21,6 +21,8 @@
 
 package com.sun.corba.ee.impl.util;
 
+import com.sun.corba.ee.impl.misc.ConcurrentSoftCache;
+
 import com.sun.corba.ee.impl.io.ObjectStreamClass;
 import com.sun.corba.ee.impl.javax.rmi.CORBA.Util;
 import com.sun.corba.ee.impl.misc.ClassInfoCache ;
@@ -34,7 +36,6 @@
 import java.util.Map;
 import java.util.WeakHashMap;
 
-import org.glassfish.pfl.basic.concurrent.SoftCache;
 
 public class RepositoryId {
 
@@ -77,7 +78,14 @@ public class RepositoryId {
     private static final Map<Class<?>, String> classSeqToRepStr = new WeakHashMap<>();
 
     private static final Map<String, byte[]> repStrToByteArray = new IdentityHashMap<>();
-    private static final Map<String, Class<?>> repStrToClass = new SoftCache<>();
+    /*
+     * Read by getAnyClassFromType with no lock at all, while the writes below
+     * happen under the classToRepStr monitor. With the previous SoftCache -
+     * a bare HashMap that mutates itself even inside get() - that was a data
+     * race, and two concurrent readers were enough to corrupt it.
+     */
+    private static final ConcurrentSoftCache<String, Class<?>> repStrToClass =
+        new ConcurrentSoftCache<>();
 
     private String repId = null;
     private boolean isSupportedFormat = true;
@@ -779,6 +787,7 @@ public static String createForJavaType(java.io.Serializable ser)
                 createHashString(clazz);
 
             classToRepStr.put(clazz, repid);
+            repStrToClass.purge();
             repStrToClass.put(repid, clazz);
             return repid;
         }
@@ -815,6 +824,7 @@ public static String createForJavaType(Class<?> clz, ClassInfoCache.ClassInfo ci
                 createHashString(clz);
 
             classToRepStr.put(clz, repid);
+            repStrToClass.purge();
             repStrToClass.put(repid, clz);
             return repid;
         }

orbmain/src/main/java/com/sun/corba/ee/impl/util/RepositoryIdCache.java

@@ -20,18 +20,35 @@
 
 package com.sun.corba.ee.impl.util;
 
-import java.util.Hashtable;
+import java.util.concurrent.ConcurrentHashMap;
 
-public class RepositoryIdCache extends Hashtable<String, RepositoryId> {
-    public final synchronized RepositoryId getId(String key) {
-        RepositoryId repId = super.get(key);
+/**
+ * Interns {@link RepositoryId} instances by their string form.
+ *
+ * <p>This was a {@code Hashtable} with a {@code synchronized getId} on top,
+ * held in a static field on {@link RepositoryId}, so every value type
+ * marshalled anywhere in the process took the same monitor twice - once for
+ * the method and once inside the Hashtable itself. The map is now concurrent
+ * and the hit path takes no lock.
+ */
+public class RepositoryIdCache extends ConcurrentHashMap<String, RepositoryId> {
 
+    private static final long serialVersionUID = 1L;
+
+    /**
+     * @param key the repository id string
+     * @return the interned RepositoryId for that string, creating one if this
+     *         is the first time it has been seen
+     */
+    public final RepositoryId getId(String key) {
+        // Look before computing: computeIfAbsent locks a bin even when the key
+        // is present, and present is what almost every call finds.
+        RepositoryId repId = get(key);
         if (repId != null) {
             return repId;
-        } else {
-            repId = new RepositoryId(key);
-            put(key, repId);
-            return repId;
         }
+        // RepositoryId's constructor only parses the string it is given, so it
+        // cannot re-enter this map - which computeIfAbsent would not tolerate.
+        return computeIfAbsent(key, RepositoryId::new);
     }
 }