Parallelize node/edge encoding in serializeGraphStore - #291
Merged
Conversation
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.
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.
2 tasks
Member
Author
|
Ran an benchmark on the |
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.
Summary
serializeNode/serializeEdgeand the main write loop now call type-specific writers directly instead of routing every scalar field through the generic ~110-branchserialize(DataOutput, Object)dispatcher. Byte-identical by construction; reduces per-element overhead that would otherwise be multiplied across worker threads.serializeGraphStorenow fans node/edge encoding out across a per-callExecutorServicewhenever a store spans more than one internal storage block, reusingNodeStore/EdgeStore's existing lock-free, block-boundary-awareSpliterator(already backingparallelStream()) to split work without touching any locking. Results are drained back to the output stream through a bounded in-flight window (not a single blockinginvokeAll) so peak memory stays bounded instead of materializing the whole payload at once. Stores with a single block (small graphs, all existing golden fixtures) take the exact same sequential path as before.GraphModel.Serialization.write(DataOutput, GraphModel)keeps its signature and produces byte-identical output for a given graph.Test plan
testParallelAndSequentialNodeEdgeEncodingProduceIdenticalBytes: byte-equality between the sequential and parallel encode paths on a multi-block graph with scattered garbage (removed) slots.testSerializeAndDeserializeMultiBlockGraphRoundTrips: end-to-end round-trip through the publicserializeGraphStore/deserializeGraphStoreon a multi-block graph, confirming the parallel path is actually exercised.testSerializeChunksInParallelPropagatesExceptionAndLeavesNoThreads: a failure mid-chunk surfaces as the expected exception and leaves no leaked worker threads.Note: while writing the round-trip test I found a pre-existing, unrelated bug — a graph that's both multi-block and has scattered node/edge removals fails to round-trip with "The edge source or target can't be found," reproducible on the pre-existing sequential code too. Tracking that separately; it's not introduced by this change and this PR's tests route around it.
🤖 Generated with Claude Code