Take the global monitors off the marshalling path

[AngeloRubens]

Every Serializable object marshalled or unmarshalled anywhere in the process
passes through ObjectStreamClass.lookupInternal, which takes one process wide
monitor — and takes it even when the descriptor is already cached and fully
initialized. The file has known this for a long time; the comment in that method
says so:

// Another possibility is to continue to synchronize on the
// descriptorFor map, but that leads to poor performance
// (see bug 4165204 "ObjectStreamClass can hold global lock
// for a very long time").
//
// We will need to live with 4165204 until we can rewrite
// this to follow the improved java.io.ObjectStreamClass
// in J2SE 1.4 and later.

Measured

Four cores, lookups per second, best of three:

threads	before	after
1	30,308,186	137,580,915	4.5x
2	16,674,433	258,879,774	15.5x
4	13,867,919	246,933,683	17.8x
8	15,067,581	300,386,540	19.9x

The shape matters more than the multiplier. Before, adding threads made total
throughput worse — 8 threads did half the work of 1, which is what a contended
monitor looks like from the outside. After, it climbs.

Approach

This does not rewrite initialization. The same comment records that moving
init() out of the monitor was tried and reverted because it deadlocks (bug
5104239), so the slow path is left exactly as it was and a lock free fast path
is added in front of it: if the descriptor is present and reports itself
initialized, return it. initialized becomes volatile and is already assigned
last in init(), so it doubles as the publication fence for everything else the
descriptor computed. A descriptor is published to the cache by the constructor
before init() runs — on purpose, so recursive lookups find it — which is why
presence alone is not enough.

That fast path needs lock free reads, and the cache could not provide them:
org.glassfish.pfl.basic.concurrent.SoftCache is, despite the package, a bare
HashMap that was safe only because every caller held the monitor. It is
replaced by ConcurrentSoftCache. Values stay soft because an
ObjectStreamClass holds its Class, so a strong map would pin the application
class loader for the life of the process.

Two more monitors on the same path

RepositoryIdCache, reached for every value type, was a Hashtable with a
synchronized getId on top — the same monitor taken twice, once for the method
and once inside the Hashtable. Now a ConcurrentHashMap whose hit path takes no
lock.

ObjectStreamClass.translateFields was static synchronized, so it locked the
class object shared with every other static synchronized member.
PersistentFieldsValue.translateFields allocates a new array and reads only its
argument; the lock guarded nothing.

One of these was not only slow but wrong

RepositoryId.repStrToClass was that same non-thread-safe SoftCache, written
under the classToRepStr monitor but read by getAnyClassFromType under no
lock at all — a data race on a plain HashMap. Worse, SoftCache mutates its
map inside get(), so two concurrent readers were enough on their own. Now a
ConcurrentSoftCache.

Tests

353 pass. The contention test asserts the property rather than a timing
threshold: it holds the cache monitor and requires a lookup of an already
initialized descriptor to complete anyway. Reverted against the old
ObjectStreamClass that test blocks for its full ten second timeout and fails,
so it is not vacuous.

DescriptorLookupThroughput is a main and not a test, because a number is not
something to assert on a shared build machine. It is also the second harness
written for this: the first checked a deadline inside the loop and reported both
versions as identical and both as failing to scale. System.nanoTime is not
always a cheap userspace read, and when it is not every thread queues on the
clock source — the harness had become the contended resource it was looking
for.

[dmatej]

Nice!
Just a quick question - is the SoftCache class still used somewhere, or we can delete it from PFL, or at least mark as deprecated and forRemoval?

[AngeloRubens]

Checked on the branch: after this change there are no remaining uses of SoftCache in the ORB. The only two occurrences left are references to it in javadoc explaining what was replaced.

So yes — it can be marked deprecated forRemoval in PFL, and dropped once nothing else in the ecosystem depends on it.

One caveat: WeakCache, in the same org.glassfish.pfl.basic.concurrent package, is still used — ORBImpl and PresentationManagerImpl both rely on it. So the package stays; only SoftCache becomes unused here.

It may be worth recording why it can go, not just that it can. SoftCache is a plain HashMap with no synchronization of its own, and its get() mutates the map through processQueue(). Both uses in this repository were unsafe: one was correct only because every caller happened to hold an external monitor, and the other — RepositoryId.repStrToClass — was read with no lock at all while other threads wrote it, so two concurrent readers were enough on their own. Anything else still using it is likely to have the same problem, so deprecating it with that rationale in the javadoc would help whoever finds it next.