bitflicker64 opened a new pull request, #3175:
URL: https://github.com/apache/hugegraph/pull/3175
## Purpose of the PR
Follow-up to #2859 (original work by @javeme), targeting its branch
`only-one-id-query-optimize` so it can be merged into that PR. It replaces the
implementation in that branch with a smaller one, fixes two semantic
differences from master, and adds the tests and numbers that were asked for in
the review. The author has been inactive on it since 2025-09 and it was
relabeled `help wanted`. Note that #2859 itself still needs a rebase onto
master (it conflicts with #2982 in `queryEdgesByIds`); the implementation here
is written so that rebase is trivial, since the multi-id path and the
`distinctIds` line are left untouched.
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 the current #2859 code, 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) with the same change
applied on top of current master: `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). On this branch
the same delta applies cleanly on top of the #2859 base; `VertexCoreTest` and
`EdgeCoreTest` are being re-run here and I will update this section with the
result. `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]
