perf(core): optimize queryVerticesByIds and queryEdgesByIds for one id - #3174
Closed
bitflicker64 wants to merge 1 commit into
Closed
perf(core): optimize queryVerticesByIds and queryEdgesByIds for one id#3174bitflicker64 wants to merge 1 commit into
bitflicker64 wants to merge 1 commit into
Conversation
Querying a vertex or an edge by a single id is the hot path behind graph.vertex(id), graph.vertices(id), graph.edge(id) and the adjacent vertex loading in HugeVertex.ensureFilledProperties(). It went through the general multi-id code and allocated an id list, a HashMap and a MapperIterator on every call. Add a dedicated single-id path for vertices and edges that keeps the semantics of the multi-id path (removed, updated, expired and missing records, null and invalid ids, IN direction edge ids, undefined adjacent vertices, NotFoundException when the record must exist) but only does the map lookups and, on a miss, one OneIdQuery against the backend. The multi-id path is unchanged. The "vertex not found" handling (NotFoundException, undefined adjacent vertex, or skip) is shared by both paths. Supersedes apache#2859, original work by Jermy Li (javeme). Co-authored-by: Jermy Li <jermy@apache.org>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3174 +/- ##
============================================
- Coverage 37.72% 32.66% -5.07%
+ Complexity 6521 5501 -1020
============================================
Files 800 789 -11
Lines 68821 67744 -1077
Branches 9127 8958 -169
============================================
- Hits 25965 22126 -3839
- Misses 39815 43013 +3198
+ Partials 3041 2605 -436 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Closing this one, will re-open it against the branch of #2859 instead. |
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.
Purpose of the PR
Supersedes #2859, original work by @javeme (rebased onto current master, semantics fixed, tests and numbers added). The author has been inactive on it since 2025-09 and it was relabeled
help wanted.Querying a vertex or an edge by a single id is the hot path behind
graph.vertex(id),graph.vertices(id),graph.edge(id),graph.adjacentVertex(id)and the lazy adjacent-vertex loading inHugeVertex.ensureFilledProperties(). It currently goes through the general multi-id code inGraphTransaction.queryVerticesByIds()/queryEdgesByIds()and allocates anIdQuery, an id list, aHashMapand aMapperIteratoron every call.Main Changes
GraphTransaction: add a dedicated single-id path,queryVertexById()andqueryEdgeById(), taken when exactly one id is passed. It does the three local-tx map lookups (only when the tx has changes) and, on a miss, a singleIdQuery.OneIdQueryagainst the backend. The multi-id path is unchanged.NotFoundException/ undefined adjacent vertex / skip) is shared by both paths throughresolveVertex().Differences from #2859, on purpose:
adjacentVertex(id)for a vertex deleted in the current tx returnedHugeVertex.undefined(...)instead of nothing. With the defaultvertex.check_adjacent_vertex_exist=falsethat silently relabels the other vertex of a held edge to~undefinedand wipes its properties on the next property access. This PR keeps master's behaviour (nothing is returned), identical to the multi-id path. Covered bytestQueryAdjacentVertexRemovedInLocalTx,testQuerySingleVertexByIdRemovedInLocalTxand the two...ExpiredInLocalTxtests.QueryResults.one(), which throws if the backend yields two results, while master returns the backend iterator as is. Every backend answers an exact edge id with at most one column (memoryentry.contains(column), RocksDBgetById, HStoregetById) andqueryEdgesFromBackendInternalalready asserts that, soone()would most likely be safe, but there is no reason to change the contract in a perf PR: this PR returns the backend iterator directly, exactly like the multi-id path does for one id with no local hit. Note that this is guaranteed by construction (queryEdgeById()never callsone()), not by a test: no shipped backend can be made to yield two edges for one exact id, so a test would need a stubbedqueryEdgesFromBackend(). For vertices the single-id backend path usesQueryResults.one(), which is whatCachedGraphTransaction.queryVerticesByIds()already does today for every single-id query when the vertex cache is enabled (the default).@Watchedannotations, theprefix = "tx"to"graph"change onprepareCommit()(which would diverge from the 13prefix = "tx"sites inAbstractTransaction), and theoptimizeQuery()switch toOneIdQueryfor primary-key lookups. They are unrelated to this optimization and can be separate PRs.asserts flagged by Copilot on perf(core): optimize queryVerticesByIds for only-one-id query #2859 are gone with the restructuring, and the boolean-flag control flow @imbajin commented on is replaced by early returns in the dedicated methods. I did not extract a sharedfindVertexInLocalTx()helper: the lookup has three outcomes (found, removed or expired, not tracked) so a helper would need a tri-state result or a second lookup, and folding the multi-id loops into it would touch code this PR otherwise leaves alone. If you would still like that, I am happy to do it here or as a follow-up.distinctIdscheck from refactor(server): optimize rockdb batch query perf #2982:[a, b, a]still yields three edges (testQueryEdgesByNonConsecutiveDuplicateIds), and the same is now asserted for vertices and for mixed local/backend ids.Benchmark
JMH, memory backend with default cache settings, 1000 vertices in a chain, single thread, 1 fork, 3 warmup + 5 measurement iterations of 1 s, Apple M-series, OpenJDK 11.0.31.
clean= no uncommitted change in the tx,dirty= one uncommitted vertex and edge in the tx. Lower is better (ns/op).vertexByIdCommitted(graph.vertices(id), cached vertex)vertexByIdCommitted(graph.vertices(id), cached vertex)vertexByIdStrict(graph.vertex(id))vertexByIdStrict(graph.vertex(id))adjacentVertexById(graph.adjacentVertex(id))adjacentVertexById(graph.adjacentVertex(id))vertexByIdMissing(graph.vertices(id), id not found)vertexByIdMissing(graph.vertices(id), id not found)vertexByIdLocalTx(graph.vertices(id), id added in tx (dirty only))edgeByIdCommitted(graph.edges(id), cached edge)edgeByIdCommitted(graph.edges(id), cached edge)edgeByIdLocalTx(graph.edges(id), id added in tx (dirty only))verticesByTwoIds(graph.vertices(id1, id2), multi-id path, unchanged)verticesByTwoIds(graph.vertices(id1, id2), multi-id path, unchanged)The single-id vertex lookups drop from roughly 80 to 100 ns to 22 to 30 ns, mostly by not allocating an
IdQuery, a list, aHashMapand aMapperIteratorper call. A cached edge lookup is dominated byEdgeIdparsing and the edge cache key, so it only gains 7 to 18 percent; the local-tx edge hit gains the most. The multi-id path is within noise, as expected.The harness is
QueryByIdBenchmark(not part of this PR, happy to add it underhugegraph-test/src/test/java/org/apache/hugegraph/benchmarkif wanted).Verifying these changes
VertexCoreTest:testQuerySingleVertexByIdInLocalTx(added and updated),testQuerySingleVertexByIdRemovedInLocalTx,testQuerySingleVertexByIdNotFound,testQuerySingleVertexByNullId,testQuerySingleVertexByIdExpiredInLocalTx,testQueryVerticesByNonConsecutiveDuplicateIds,testQueryVerticesByIdsWithLocalAndDuplicateIdsEdgeCoreTest:testQuerySingleEdgeByIdInLocalTx,testQuerySingleEdgeByIdRemovedInLocalTx,testQuerySingleEdgeByIdNotFound,testQuerySingleEdgeByInvalidId,testQuerySingleEdgeByIdWithInDirection,testQuerySingleEdgeByIdExpiredInLocalTx,testQueryEdgesByIdsWithLocalAndDuplicateIds,testQueryAdjacentVertexRemovedInLocalTxmvn test -pl hugegraph-server/hugegraph-test -am -P core-test,memory,CoreTestSuite: 819 run, 0 failures, 0 errors, 94 skipped (the usual "Not support paging" / hstore-only skips).UnitTestSuitewas not run in full;CachedGraphTransactionTest(the cache override of the single-id backend query): 13 run, 0 failures. Checkstyle (mvn validate) passes with 0 violations.Does this PR potentially affect the following parts?
Documentation Status
Doc - No Need