Add timed acquisition and queue diagnostics to GraphLock - #294
Open
MattArtzAnthro wants to merge 278 commits into
Open
Add timed acquisition and queue diagnostics to GraphLock#294MattArtzAnthro wants to merge 278 commits into
MattArtzAnthro wants to merge 278 commits into
Conversation
…tion in IndexImpl
…ype edge counts when parallel edges exist
…ting the Collection contract
… all live views with stale bit-vectors, counts, and type arrays
…iew spliterators throws IllegalStateException: Accept exceeded fixed size
….6 (gephi#275) Bumps [org.apache.maven.plugins:maven-surefire-plugin](https://github.com/apache/maven-surefire) from 3.5.5 to 3.5.6. - [Release notes](https://github.com/apache/maven-surefire/releases) - [Commits](apache/maven-surefire@surefire-3.5.5...surefire-3.5.6) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-surefire-plugin dependency-version: 3.5.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.sonatype.central:central-publishing-maven-plugin](https://github.com/sonatype/central-publishing-maven-plugin) from 0.10.0 to 0.11.0. - [Commits](https://github.com/sonatype/central-publishing-maven-plugin/commits) --- updated-dependencies: - dependency-name: org.sonatype.central:central-publishing-maven-plugin dependency-version: 0.11.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/checkout](https://github.com/actions/checkout) from 6 to 7. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v6...v7) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '7' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.jacoco:jacoco-maven-plugin](https://github.com/jacoco/jacoco) from 0.8.14 to 0.8.15. - [Release notes](https://github.com/jacoco/jacoco/releases) - [Commits](jacoco/jacoco@v0.8.14...v0.8.15) --- updated-dependencies: - dependency-name: org.jacoco:jacoco-maven-plugin dependency-version: 0.8.15 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [it.unimi.dsi:fastutil](https://github.com/vigna/fastutil) from 8.5.18 to 8.5.19. - [Changelog](https://github.com/vigna/fastutil/blob/master/CHANGES) - [Commits](https://github.com/vigna/fastutil/commits/8.5.19) --- updated-dependencies: - dependency-name: it.unimi.dsi:fastutil dependency-version: 8.5.19 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) * Harden serialization: fail on unknown tags, fix char encoding Three scoped fixes to the serialization layer. No change to the on-disk format, so no VERSION bump. - deserialize() silently returned null for an unrecognized type tag, surfacing later as a confusing ClassCastException or silent data loss. It now throws IOException naming the tag. The preceding `case -1` was unreachable (readUnsignedByte never returns -1) and is removed. - CHAR and CHAR_ARRAY relied on DataOutput.writeChar/readChar, but DataInputOutput implements those with 4 bytes instead of the 2 the interface specifies. Production writes via DataOutputStream, so no stored data is affected, but graphstore's own tests were round-tripping an encoding that never reaches disk. Both sides now use writeShort/readUnsignedShort, which is byte-identical to DataOutputStream.writeChar, and DataInputOutput.writeChar/readChar are fixed to honour the contract. - Dropped Locale support. Locale is not an AttributeUtils supported type, so it cannot enter a graph through the public API. Tag 124 is kept reserved so it is never reused. Adds a test asserting all serialization tag constants are distinct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cover non-ASCII chars in serialization tests The char tests only used ASCII values, so they could not detect a narrowing of the 2-byte encoding. Extend them across the boundaries of the 16-bit range: above 0x7F, above 0x7FF, either side of the signed-short flip, the 16-bit maximum, and an unpaired surrogate. Verified by temporarily narrowing CHAR to a symmetric 1-byte encoding, which the previous values did not catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
GraphAttributesImpl backed its map with a java.util.HashMap, and Serialization.serializeGraphAttributes iterates that map's entrySet() straight into the byte stream. HashMap iteration order is an implementation detail, so the serialized bytes were a function of insertion history rather than of content alone. Switch the field to a TreeMap so iteration is sorted by key and the output bytes become a pure function of the content. This is a prerequisite for byte-pinned serialization fixtures. TreeMap rejects null keys where HashMap accepted them, and throws NPE from deep inside the map on get(null)/containsKey(null) where HashMap returned null/false. Add explicit Objects.requireNonNull(key, "key") guards to every public method taking a key so the failure is intentional and well-messaged. deepHashCode and deepEquals are unchanged and remain correct: Map.hashCode() is specified as the order-independent sum of entry hash codes, and deepEquals goes through MapDeepEquals which compares by key lookup. The read path is unaffected -- deserializeGraphAttributes just puts entries into the map -- so previously written files still load identically. Only the order of newly written graph-attribute entries changes; the format itself is untouched and Serialization.VERSION is not bumped. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Add golden-fixture regression suite for the serialization format Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Rebuild the time index on read instead of restoring it TimeIndexStore.countMap came back doubled after a load. The time store is read before the nodes and edges, which restored the reference counts from the stream, and inserting the elements then re-indexed their time references and incremented the same counts again. Counts never reached zero on remove, so time values stayed in the index once nothing referenced them. The index is derived state: nothing outside the block references its slot ids, and TimeIndexStore.index/clear already maintain one count per element reference. Reading now parses the block to advance the stream and discards it, letting element insertion rebuild the index. Loading a store whose counts were inflated by ElementImpl.setTimeAttribute yields canonical counts and drops time values nothing references. The write path is unchanged, so the byte format and the golden fixtures stay as they are. testTimestampStore and testIntervalStore asserted that the block round-trips its own state with no elements present. They now assert it is consumed in full and carries nothing. * Count one time index reference per dynamic attribute time ElementImpl.setTimeAttribute passed the whole map to updateIndex, so each put re-counted every time already in it, including puts that only overwrote an existing time. removeTimeAttribute decrements one at a time, so the counts drifted upward without bound and time values stayed in the index once nothing referenced them. AttributesImpl.setAttribute now reports whether the time was new, and setTimeAttribute passes that single time to the index, matching addTime and removeTimeAttribute. Dynamic attribute columns are never value indexed, so the column index is unaffected by the narrower value. The counts written to disk are now canonical, which moves one byte in each of the two 0.8 fixtures holding dynamic times. The layout is unchanged and the field is ignored on read. * Remove unecessary comment * Reference gephi#288 in the rebuild-on-read test * Write the time index block empty The block is derived state: reading rebuilds it from the nodes and edges. The layout is kept, since earlier versions read it positionally, and the fields are written empty. Serialized bytes no longer depend on slot allocation history, so the same content always serializes to the same bytes. The deserializers read each field through its declared type again, so a mismatched block fails at the offending field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Cover the empty index block and re-setting a held time Assert that index content leaves no trace in the serialized bytes, and set a time the dynamic map already holds so the round-trip count comparison exercises the write path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Document that mutating a live TimeMap or TimeSet bypasses the index getAttribute returns the instance held by the element. The types carry no reference to the store, so putting or removing on one directly leaves the time index stale. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fold the empty index block assertion into the store tests testTimestampStore and testIntervalStore already built an index with content and serialized it, so they cover the write side too. Drops the two separate tests and the assertions that restated isEmpty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Hold the graph read lock for the entire duration of serializeGraphStore Previously the only locking came incidentally from NodeStore/EdgeStore iterators, which release the lock between the node and edge loops and leave the configuration/columns/time store/views sections unprotected, so a serialized graph could observe a torn state under concurrent mutation. * Hold the graph write lock for the entire duration of deserializeGraphStore deserializeNode/deserializeEdge write directly into NodeStore/EdgeStore, bypassing GraphStore's own auto-locked addNode()/addEdge(), so deserialization had no lock coverage at all and could race with a concurrent reader on the same graph model. * Cleanup
A file written by a newer graphstore than the one reading it currently fails deep inside deserialize()'s switch with a generic "Unknown serialization type tag" once it hits an unrecognized tag, after the store has already been locked/partially mutated. Check the version immediately after it's read, before touching any state, and raise a dedicated UnsupportedFormatVersionException (extends IOException, so no signature changes) that callers like Gephi can catch specifically to show a clean, localized message instead of a generic I/O error. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…hi#292) * Fix GraphGenerator.generateLargeGraphStore() building edges over detached nodes generateLargeEdgeList() creates edges against its own independently- sized, throwaway NodeStore rather than the real one - so edge.source/ target only lined up with generateLargeGraphStore()'s actual inserted nodes by numeric coincidence (matching storeId, different objects). removeNode()'s cascade-edge-removal walks the real node's own adjacency links, which were never wired to these edges, so removing a referenced node silently left a dangling edge behind instead of cascading. That only surfaced once something both spanned multiple storage blocks and had elements removed afterward - traced back to plain, pre-existing sequential serialization code, not anything specific to threading. Root cause confirmed directly: graphStore.nodeStore.get(edge.source. storeId) != edge.source for every edge before this fix. Fix: build edges via generateEdgeList(graphStore.nodeStore, ...) so they reference the real nodes. generateLargeNodeList()/ generateLargeEdgeList() are left untouched since other tests use them independently; generateLargeGraphStore() was unused before this session's tests, so nothing depended on the old behavior. * Move the regression test to a dedicated GraphGeneratorTest The fix touches GraphGenerator, not Serialization, so the test should exercise that directly rather than proving it indirectly through a full serialize/deserialize round-trip. Asserts the actual invariant that broke: every edge's source/target is the same object registered in the store's own nodeStore, not just one with a matching storeId. Confirmed red on the pre-fix generator, green after. * Remove explanatory comment from generateLargeGraphStore()
* Use direct typed writes for node/edge serialization serializeNode/serializeEdge and the main write loop routed every scalar field (storeId, edge type, weight, directed flag, properties) through the generic ~110-branch serialize(DataOutput, Object) dispatcher. Call the type-specific writers directly instead - byte-identical by construction, and removes a per-element dispatch cost that would otherwise be multiplied across worker threads once serialization is parallelized. * Parallelize node/edge encoding in serializeGraphStore NodeStore/EdgeStore already have a lock-free, block-boundary-aware Spliterator (backing parallelStream()) that splits at storage block boundaries and skips garbage slots. Use it to fan node/edge encoding out across a per-call thread pool whenever a store spans more than one block, and drain results back to the output stream through a bounded in-flight window (not invokeAll) so peak memory stays bounded instead of materializing the whole payload at once. Below the single-block threshold - which trySplit() reports on its own - encoding stays on the calling thread with the exact same code path, so small graphs are unaffected. Output bytes are unchanged either way: no shared mutable state exists on the write path, so concatenating independently-encoded chunks in original block order reproduces today's exact serialization format.
…i#293) FormattingAndParsingUtils.parseValue() stopped unquoted value parsing at ')' as well as ']', a rule intended for interval bounds like "(1,2)". ArraysParser reused the same method for plain string/array elements, so a literal ')' inside an unquoted array element (e.g. liststring value "[Foo,Bar(Foo)]") was misread as the end of the value and silently dropped. Arrays are only ever delimited by '[', ']' and ',', so '(' and ')' now have no structural meaning there and are kept as part of the value. Fixes gephi/gephi#2989
GraphLock exposed only unbounded, non-interruptible lock() calls, so a caller that cannot afford to wait indefinitely had no option in the public API. This adds tryReadLock and tryWriteLock with a timeout, plus getReadLockCount, isWriteLocked, and getQueueLength for monitoring, each delegating to the underlying ReentrantReadWriteLock. New methods are default methods on the interface so existing implementations keep compiling. Javadoc on readLock now states the consequence of holding a read lock across a cross-thread wait or abandoning an auto-locking iterator. Fixes gephi#282.
MattArtzAnthro
force-pushed
the
graphlock-timed-acquisition
branch
2 times, most recently
from
August 28, 2026 19:43
ca07fe7 to
890e9e7
Compare
7 tasks
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
3 participants
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.
Summary
GraphLockexposed only unbounded, non-interruptiblereadLock()/writeLock()calls, so a caller that cannot afford to wait indefinitely (a plugin doing graph work off the EDT while the viz engine renders) had no option in the public API short of reflecting intoGraphLockImpl.tryReadLock(timeout, unit)andtryWriteLock(timeout, unit), delegating to the underlyingReentrantReadWriteLock's timedtryLock. The timed form enqueues rather than barging, which is why it did not starve in the reproducer on raphLock has no timed/try acquisition; polling writers can starve, and a stalled reader plus a queued writer can wedge all graph operations #282 where untimedtryLock()polling did (0 of 4637 attempts vs 2116 of 2116).tryWriteLockperforms the same read-hold check aswriteLock().getReadLockCount(),isWriteLocked(), andgetQueueLength()for monitoring.getReadLockCount()counts holds across all threads, which is what makes a leaked read hold (an abandoned auto-locking iterator) diagnosable; today it is invisible to a thread dump once the holding thread has exited.defaultmethods on the interface, throwingUnsupportedOperationException, so any external implementation keeps compiling. Happy to make them abstract instead if you prefer, sinceGraphLockImplis the only implementation in this repository.readLock()now states that a read hold across a cross-thread wait, or an iterator abandoned before exhaustion ordoBreak(), blocks all graph operations once a writer queues. No existing behavior changes; fairness is untouched.Fixes #282
Test plan
GraphLockImplTest: timed read and write acquire when free; time out under another thread's write and read respectively, then acquire after release;tryWriteLockthrowsIllegalMonitorStateExceptionwhen the caller holds a read lock; interruption during a timed wait propagates and leaves no hold;getQueueLengthreports a queued writer;getReadLockCountcounts holds from two threads whilegetReadHoldCountstays per-thread;isWriteLockedfollows lock and unlock. Contention is set up with latches, not sleeps, apart from a bounded poll on the queue length.mvn -B packagepasses the full suite (1669 tests) with the formatter applied.As with the issue, this was worked out together with Claude, Anthropic's AI assistant; I built and ran the tests on my machine.