bitflicker64 opened a new pull request, #3174:
URL: https://github.com/apache/hugegraph/pull/3174
## 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 in
`HugeVertex.ensureFilledProperties()`. It currently goes through the general
multi-id code in `GraphTransaction.queryVerticesByIds()` / `queryEdgesByIds()`
and allocates an `IdQuery`, an id list, a `HashMap` and a `MapperIterator` on
every call.
## Main Changes
- `GraphTransaction`: add a dedicated single-id path, `queryVertexById()`
and `queryEdgeById()`, 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 single
`IdQuery.OneIdQuery` against the backend. The multi-id path is unchanged.
- The "vertex not found" handling (`NotFoundException` / undefined adjacent
vertex / skip) is shared by both paths through `resolveVertex()`.
Differences from #2859, on purpose:
1. **Removed or expired records in the tx.** #2859 put the id into the
result id list before checking the removed/expired maps, so
`adjacentVertex(id)` for a vertex deleted in the current tx returned
`HugeVertex.undefined(...)` instead of nothing. With the default
`vertex.check_adjacent_vertex_exist=false` that silently relabels the other
vertex of a held edge to `~undefined` and wipes its properties on the next
property access. This PR keeps master's behaviour (nothing is returned),
identical to the multi-id path. Covered by
`testQueryAdjacentVertexRemovedInLocalTx`,
`testQuerySingleVertexByIdRemovedInLocalTx` and the two `...ExpiredInLocalTx`
tests.
2. **Single edge id from the backend.** #2859 routed it through
`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 (memory `entry.contains(column)`, RocksDB `getById`,
HStore `getById`) and `queryEdgesFromBackendInternal` already asserts that, so
`one()` 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 calls `one()`), not by a
test: no shipped backend can be made to yield two edges for one exact id, so a
test would need a stubbed `queryEdgesFromBackend()`. For vertices the single-id
backend path uses `QueryResults.one()`, which is what
`CachedGraphTransaction.queryVerticesByIds()` already does today for every
single-id query when the v
ertex cache is enabled (the default).
3. Not included: the 19 `@Watched` annotations, the `prefix = "tx"` to
`"graph"` change on `prepareCommit()` (which would diverge from the 13 `prefix
= "tx"` sites in `AbstractTransaction`), and the `optimizeQuery()` switch to
`OneIdQuery` for primary-key lookups. They are unrelated to this optimization
and can be separate PRs.
4. The two redundant `assert`s flagged by Copilot on #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 shared
`findVertexInLocalTx()` 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.
5. The multi-id edge path keeps the `distinctIds` check from #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).
| Benchmark | tx | master (ns/op) | this PR (ns/op) | change |
|---|---|---:|---:|---:|
| `vertexByIdCommitted` (graph.vertices(id), cached vertex) | clean | 85.5 ±
0.9 | 29.0 ± 4.1 | -66% |
| `vertexByIdCommitted` (graph.vertices(id), cached vertex) | dirty | 83.5 ±
4.5 | 29.9 ± 8.6 | -64% |
| `vertexByIdStrict` (graph.vertex(id)) | clean | 84.2 ± 5.7 | 22.5 ± 0.7 |
-73% |
| `vertexByIdStrict` (graph.vertex(id)) | dirty | 75.0 ± 0.2 | 25.7 ± 0.9 |
-66% |
| `adjacentVertexById` (graph.adjacentVertex(id)) | clean | 97.1 ± 4.8 |
22.3 ± 0.5 | -77% |
| `adjacentVertexById` (graph.adjacentVertex(id)) | dirty | 80.7 ± 32.1 |
26.9 ± 0.3 | -67% |
| `vertexByIdMissing` (graph.vertices(id), id not found) | clean | 189.5 ±
8.4 | 116.8 ± 5.1 | -38% |
| `vertexByIdMissing` (graph.vertices(id), id not found) | dirty | 199.1 ±
25.3 | 110.6 ± 7.7 | -44% |
| `vertexByIdLocalTx` (graph.vertices(id), id added in tx (dirty only)) |
dirty | 47.3 ± 2.3 | 31.3 ± 37.5 | -34% |
| `edgeByIdCommitted` (graph.edges(id), cached edge) | clean | 491.8 ± 15.1
| 459.0 ± 142.5 | -7% |
| `edgeByIdCommitted` (graph.edges(id), cached edge) | dirty | 549.0 ± 39.2
| 449.3 ± 27.8 | -18% |
| `edgeByIdLocalTx` (graph.edges(id), id added in tx (dirty only)) | dirty |
94.4 ± 7.4 | 19.2 ± 4.3 | -80% |
| `verticesByTwoIds` (graph.vertices(id1, id2), multi-id path, unchanged) |
clean | 184.5 ± 8.6 | 176.9 ± 11.7 | -4% |
| `verticesByTwoIds` (graph.vertices(id1, id2), multi-id path, unchanged) |
dirty | 189.5 ± 2.1 | 183.7 ± 20.4 | -3% |
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, a `HashMap` and a
`MapperIterator` per call. A cached edge lookup is dominated by `EdgeId`
parsing 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
under `hugegraph-test/src/test/java/org/apache/hugegraph/benchmark` if wanted).
## Verifying these changes
- [x] Need tests and can be verified as follows:
- `VertexCoreTest`: `testQuerySingleVertexByIdInLocalTx` (added and
updated), `testQuerySingleVertexByIdRemovedInLocalTx`,
`testQuerySingleVertexByIdNotFound`, `testQuerySingleVertexByNullId`,
`testQuerySingleVertexByIdExpiredInLocalTx`,
`testQueryVerticesByNonConsecutiveDuplicateIds`,
`testQueryVerticesByIdsWithLocalAndDuplicateIds`
- `EdgeCoreTest`: `testQuerySingleEdgeByIdInLocalTx`,
`testQuerySingleEdgeByIdRemovedInLocalTx`, `testQuerySingleEdgeByIdNotFound`,
`testQuerySingleEdgeByInvalidId`, `testQuerySingleEdgeByIdWithInDirection`,
`testQuerySingleEdgeByIdExpiredInLocalTx`,
`testQueryEdgesByIdsWithLocalAndDuplicateIds`,
`testQueryAdjacentVertexRemovedInLocalTx`
- Ran locally on the memory backend (JDK 11): `mvn 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). `UnitTestSuite` was 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.
- Not run: RocksDB, HBase and HStore backends. The change is
backend-neutral (it only reorders which maps are consulted before the same
backend query), so CI coverage on those is what I rely on.
## Does this PR potentially affect the following parts?
- [x] Nope
## Documentation Status
- [x] `Doc - No Need`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
