imbajin commented on code in PR #3175:
URL: https://github.com/apache/hugegraph/pull/3175#discussion_r3888193138
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -784,131 +775,117 @@ protected Iterator<Vertex> queryVerticesByIds(Object[]
vertexIds, boolean adjace
return this.queryVerticesByIds(vertexIds, adjacentVertex,
checkMustExist, HugeType.VERTEX);
}
- @Watched(prefix = "graph")
protected Iterator<Vertex> queryVerticesByIds(Object[] vertexIds, boolean
adjacentVertex,
boolean checkMustExist,
HugeType type) {
Query.checkForceCapacity(vertexIds.length);
- List<Id> ids;
- Map<Id, HugeVertex> vertices;
- boolean verticesUpdated = this.verticesInTxSize() > 0;
-
if (vertexIds.length == 1) {
- Id id = HugeVertex.getIdValue(vertexIds[0]);
+ // Fast path: skip the id list, map and mapper iterator for one id
+ return this.queryVertexById(vertexIds[0], adjacentVertex,
+ checkMustExist, type);
+ }
- boolean tryQueryBackend = true;
- if (id == null) {
- tryQueryBackend = false;
- ids = ImmutableList.of();
- } else {
- ids = ImmutableList.of(id);
- }
-
- HugeVertex vertex = null;
- if (id != null && verticesUpdated) {
- if (this.removedVertices.containsKey(id)) {
- // The record has been deleted
- tryQueryBackend = false;
- } else if ((vertex = this.addedVertices.get(id)) != null ||
- (vertex = this.updatedVertices.get(id)) != null) {
- // Found from local tx
- tryQueryBackend = false;
- if (vertex.expired()) {
- vertex = null;
- } else {
- assert vertex != null;
- }
- }
- }
+ // NOTE: allowed duplicated vertices if query by duplicated ids
+ List<Id> ids = InsertionOrderUtil.newList();
+ Map<Id, HugeVertex> vertices = new HashMap<>(vertexIds.length);
- if (vertex != null) {
- assert !tryQueryBackend;
- vertices = ImmutableMap.of(vertex.id(), vertex);
- } else if (!tryQueryBackend) {
- assert vertex == null;
- vertices = ImmutableMap.of();
- } else {
- // Query from backend store
- IdQuery query = new IdQuery.OneIdQuery(type, id);
- Iterator<HugeVertex> it = this.queryVerticesFromBackend(query);
- vertex = QueryResults.one(it);
- if (vertex == null) {
- vertices = ImmutableMap.of();
- } else {
- vertices = ImmutableMap.of(vertex.id(), vertex);
- }
- }
- } else {
- // NOTE: allowed duplicated vertices if query by duplicated ids
- ids = InsertionOrderUtil.newList();
- vertices = new HashMap<>(vertexIds.length);
-
- IdQuery query = new IdQuery(type);
- for (Object vertexId : vertexIds) {
- Id id = HugeVertex.getIdValue(vertexId);
- if (id == null) {
+ IdQuery query = new IdQuery(type);
+ for (Object vertexId : vertexIds) {
+ HugeVertex vertex;
+ Id id = HugeVertex.getIdValue(vertexId);
+ if (id == null || this.removedVertices.containsKey(id)) {
Review Comment:
⚠️ This removes the `verticesUpdated` guard from the multi-ID path. On every
clean transaction, `graph.vertices(id1, id2, ...)` now calls
`removedVertices.containsKey(id)` and then up to two local-map `get`s for each
ID, whereas the previous code skipped all three maps when `verticesInTxSize()
== 0`. The PR describes the multi-ID path as unchanged, and this adds overhead
to batched lookups. Please retain the size guard around local-TX checks while
keeping the new single-ID fast path.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1012,121 +985,62 @@ public Edge queryEdge(Object edgeId) {
return edge;
}
- @Watched(prefix = "graph")
protected Iterator<Edge> queryEdgesByIds(Object[] edgeIds,
boolean verifyId) {
Query.checkForceCapacity(edgeIds.length);
- List<Id> ids;
- Map<Id, HugeEdge> edges;
- boolean edgesUpdated = this.edgesInTxSize() > 0;
-
if (edgeIds.length == 1) {
- EdgeId id = HugeEdge.getIdValue(edgeIds[0], !verifyId);
+ // Fast path: skip the id list, map and mapper iterator for one id
+ return this.queryEdgeById(edgeIds[0], verifyId);
+ }
+
+ // NOTE: allowed duplicated edges if query by duplicated ids
+ List<Id> ids = InsertionOrderUtil.newList();
+ Map<Id, HugeEdge> edges = new HashMap<>(edgeIds.length);
- boolean tryQueryBackend = true;
+ IdQuery query = new IdQuery(HugeType.EDGE);
+ for (Object edgeId : edgeIds) {
+ HugeEdge edge;
+ EdgeId id = HugeEdge.getIdValue(edgeId, !verifyId);
if (id == null) {
- tryQueryBackend = false;
- ids = ImmutableList.of();
- } else {
- if (id.direction() == Directions.IN) {
- id = id.switchDirection();
- }
- ids = ImmutableList.of(id);
- }
-
- HugeEdge edge = null;
- if (id != null && edgesUpdated) {
- if (this.removedEdges.containsKey(id)) {
- // The record has been deleted
- tryQueryBackend = false;
- } else if ((edge = this.addedEdges.get(id)) != null ||
- (edge = this.updatedEdges.get(id)) != null) {
- // Found from local tx
- tryQueryBackend = false;
- if (edge.expired()) {
- edge = null;
- } else {
- assert edge != null;
- }
- }
+ continue;
}
-
- if (edge != null) {
- assert !tryQueryBackend;
- edges = ImmutableMap.of(edge.id(), edge);
- } else if (!tryQueryBackend) {
- assert edge == null;
- edges = ImmutableMap.of();
- } else {
- // Query from backend store
- IdQuery query = new IdQuery.OneIdQuery(HugeType.EDGE, id);
- Iterator<HugeEdge> it = this.queryEdgesFromBackend(query);
- edge = QueryResults.one(it);
- if (edge == null) {
- edges = ImmutableMap.of();
- } else {
- edges = ImmutableMap.of(edge.id(), edge);
- }
+ if (id.direction() == Directions.IN) {
+ id = id.switchDirection();
}
- } else {
- // NOTE: allowed duplicated edges if query by duplicated ids
- ids = InsertionOrderUtil.newList();
- edges = new HashMap<>(edgeIds.length);
-
- IdQuery query = new IdQuery(HugeType.EDGE);
- for (Object edgeId : edgeIds) {
- HugeEdge edge;
- EdgeId id = HugeEdge.getIdValue(edgeId, !verifyId);
- if (id == null) {
+ if (this.removedEdges.containsKey(id)) {
Review Comment:
⚠️ This removes the `edgesUpdated` guard from the multi-ID path. On every
clean transaction, `graph.edges(id1, id2, ...)` now calls
`removedEdges.containsKey(id)` and then up to two local-map `get`s for each ID,
whereas the previous code skipped all three maps when `edgesInTxSize() == 0`.
The PR describes the multi-ID path as unchanged, and this adds overhead to
batched lookups. Please retain the size guard around local-TX checks while
keeping the new single-ID fast path.
--
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]
