Repository navigation
Reuse indirection tables between messages and take fewer locks per value - #339
Open
AngeloRubens wants to merge 5 commits into
Open
AngeloRubens wants to merge 5 commits into
AngeloRubens wants to merge 5 commits into
Conversation
A profile of the graph scenario (a linked list of 50 serializable nodes each way) puts CacheTable at 18.6% of server CPU and its nodes and node arrays at about 28% of the bytes allocated. A table is made per stream and filled once per message, so every value heavy request built a node per entry, a second chain for the reverse map, and grew both through 64, 128 and 256 slots. The entries now live in parallel arrays in the order they were put, and two open addressing indexes of entry numbers find them by key and by value. There is no object per entry, a lookup reads neighbouring slots instead of following references, and growing rebuilds two int arrays from the hashes already stored. Up to eight entries are still scanned linearly without hashing. A key stored with several values (readResolve, GlassFish issue 1605) is indexed by its newest entry, which links to the previous ones, so the newest value is found and a pair already present further down is still a no-op. The differential test compared against the chained version exactly, including for such keys. That version has no stable answer there: its grow() re-put each chain in the order it walked it, which reversed it, so a duplicated key gave the newest value or the oldest depending on how many times the table had grown. Nothing in the ORB can observe it - the output side, the only caller of getVal, looks a key up before storing it and so never stores one twice - but a test cannot require agreement with a coin flip. For duplicated keys and values the test now requires the newest, against an explicit model, and only that the chained version gave one of them; everywhere else it still requires exact agreement. Breaking the newest-wins rule fails four of its cases. done() now leaves the table empty and usable instead of with null arrays.
Moving the entries into arrays did not change the graph scenario's throughput, and its profile said why: the bytes allocated stayed the same, only their type changed. A table is made per stream and filled once per message, so each value heavy message still grew its arrays from nothing, copying them and rebuilding the indexes at every doubling. The cost was the regrowth, not the shape. done() now empties the table, clears the keys it held and gives its arrays to a small pool kept by the calling thread; a table that outgrows its eight small entries takes them from there, with the capacity the previous one reached. A table that stays within those eight - the arguments of most calls - never touches the pool, so the smallest messages do not pay two thread-local lookups per table for arrays they would not use. This is what serializers that track references do with their tables between messages. The pool is per thread and takes no lock. Arrays may move between threads with the streams that hold them, since once emptied they carry nothing. A table that is never done leaves its arrays to the collector, as before. Arrays above 1024 entries are not kept, so a single large message does not pin them, and a thread keeps at most eight emptied tables. CDRInputStream_1_0.close() and CDROutputStream_1_0.close() call done() on their tables. Both are called when a request ends, on the server and on the client, at the point where they already give the stream's ByteBuffer back to the pool: a closed stream is not read or written again. done() detaches the arrays from the table, so calling it twice gives nothing back twice, and a table used after it starts from new arrays rather than sharing them. The random comparison tests now end each seed with done(), so later seeds run on reused arrays; two more cover done() called twice and a small table on large reused arrays.
Every value written asks RepositoryId.createForJavaType for the ID of its class, and every call took the monitor of one static WeakHashMap, shared by all threads, to read an ID that is computed once per class. In a profile of a 50 node list it was 1.8% of server CPU at the monitor, plus WeakHashMap's expunging on each get. The IDs now live in a ClassValue, which the JDK reads without a lock and which, like the weak keys before it, lets an ID go with its class. When two threads compute the first ID of a class together, one result is kept; the IDs are equal, and repStrToClass, already concurrent, takes the same entry twice. The special cases are checked first, as before, and createSequenceRepID keeps its own lock. The test asks eight threads for the same IDs at once, first time included, and requires the known IDs from all of them.
Reading and writing a value asked ObjectStreamClassCorbaExt.isAny of the field's type string for every object field of every value, a string comparison whose answer cannot change: the type string is set when the field is made. In a profile of a 50 node list it was 2.3% of server CPU. ObjectStreamField now works it out in its constructor and both IIOPInputStream and IIOPOutputStream ask the field. The test pins the answer for the three any types and for types whose names have the same lengths, which isAny looks at first.
Every fragment written takes and releases the connection's write lock. writeLock took the stateEvent monitor just to read the state, then the writeEvent monitor to test and set a boolean, and writeUnlock took writeEvent again to clear it and notify, whether or not anybody was waiting: three monitors per fragment, so per kilobyte with GlassFish's default fragment size. In a profile of a 50 node list writeLock was 3.3% of server CPU. The state is now volatile and read without a lock, and the lock is a compare-and-set on writeLocked, so a free lock costs no monitor. A thread that finds it taken waits on writeEvent as before, still re-checking the state every 100 ms, after counting itself in writeWaiters. writeUnlock clears the flag and takes the monitor to notify only when that count is not zero. Both sides write their own volatile before reading the other's, so either the waiter's next compare-and-set sees the lock free or writeUnlock sees the waiter; the waiter counts itself and waits inside the monitor that notify needs, so the notify cannot fall between the two. The test runs eight threads through the lock 20,000 times each and requires no overlap and no lost increment - with a non-atomic take in place of the compare-and-set it sees 92 overlaps - and checks that a waiting writer gets in once the holder lets go, and that an aborted connection still refuses the lock.
Contributor
Author
|
Correction to the description: the allocation figures I first gave came from JFR's allocation samples, which are throttled and weighted estimates, and they were wrong - 24% fewer bytes on the object graph and 12% fewer on the 64 KiB string. Counted exactly from the heap summaries around each collection in the same recordings, it is 35% fewer bytes and 30% fewer collections per call on the object graph, and no change on the small call or the string. The table in the description now has those numbers as columns. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Five changes on the marshalling path, found by profiling a GlassFish server under RMI-IIOP load after #337. Each commit is self-contained, carries its own tests and its own measurement in the message.
Indirection tables (
CacheTable)done()now empties a table and gives its arrays to a small per-thread pool, and a table that outgrows its first eight entries takes them from there, the way serializers that track references keep their tables between messages. A table that stays within eight entries - most calls - never touches the pool.CDRInputStream_1_0.close()andCDROutputStream_1_0.close()calldone(), where they already give the stream'sByteBufferback: a closed stream is not read or written again. The pool takes no lock, keeps at most eight tables per thread and none above 1024 entries.The first change alone did not move throughput; its profile showed why - the same bytes were allocated, as arrays instead of nodes - and the second is what removes the regrowth.
Locks and per-value work
RepositoryId.createForJavaTypetook the monitor of one staticWeakHashMap, shared by every thread, for every value written, to read an ID computed once per class. It is aClassValuenow, read without a lock and, like the weak keys, letting an ID go with its class.anywas a string comparison repeated for every field of every value;ObjectStreamFieldworks it out once.notifywhether or not anybody waited. The state is now volatile and the lock a compare-and-set, so a free lock costs no monitor; a thread that finds it taken waits onwriteEventas before, andwriteUnlocktakes the monitor only when somebody is waiting.Measurements
GlassFish 8.0.4 on a GitHub Actions runner with the ORB jars swapped in, JDK 21 to build and JRE 25 to run. Client and server are pinned to separate cores, two each, and the server is the bottleneck. Five rounds per configuration, 15 s warm-up and 45 s measured, interleaved; medians below.
masteris eclipse-ee4j/orb master as released in 5.0.3, built in the same job. The server's CPU time is read from/procover a window inside each measurement and divided by the calls in it, which depends much less on the runner than throughput does. GlassFish's default 1 KiB fragments. Run: #37025289668, zero request errors.Master on the left of each arrow, this branch on the right. On the object graph every one of the five rounds of this branch used less CPU per call than every round of master: 183.6–188.3 µs against 191.7–199.2 µs. Allocation is counted exactly from the server's heap summaries around each collection in the JFR recordings - heap used before collection n+1 minus heap used after collection n - and divided by the calls of the same round. It falls where the indirection tables work, on the object graph; on the small call and the string the tables hold a handful of entries and nothing changes.
The harness, the test bean and the load client live on a branch of my fork, not here; each run attaches its raw output and a JFR recording of every measured run.
Limits. One four-core runner with the client and the server on it; only comparisons within a run mean anything. JFR's allocation samples are throttled and weighted estimates, and they were off by tens of percent here, which is why allocation is counted from the heap summaries instead. The JFR sampling over-attributes time to memory accesses: for the three lock and per-value changes it showed 6% fewer CPU samples on the object graph, where
/procmeasured 1.9% less CPU per call, which is why the numbers above come from/proc.Tests
mvn installon JDK 21, the whole reactor: 525 tests, none failing - 401 in orbmain, 88 in rmic, 15 in the annotation processor, 10 in internal-api, 9 in idlj, 2 in omgapi. The 22 skipped are skipped on master too. TheCacheTabletests compare it with the previous chained implementation under random operations, also on reused arrays; where that implementation gave no stable answer - itsgrow()reversed the hash chains, so a key stored twice gave the newest value or the oldest depending on how often the table had grown, which nothing in the ORB can observe because the output side looks a key up before storing it - they require the newest against an explicit model. Each new test was also run against a deliberately broken version of its change, and fails: adone()that leaves its arrays attached, a non-atomic take of the write lock (92 overlaps in 160,000 acquisitions), and so on.