javeme commented on code in PR #2704:
URL:
https://github.com/apache/incubator-hugegraph/pull/2704#discussion_r1892302685
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/traversers/KoutAPI.java:
##########
@@ -93,27 +97,40 @@ public String get(@Context GraphManager manager,
"'{}', max degree '{}', capacity '{}' and limit '{}'",
graph, source, direction, edgeLabel, depth,
nearest, maxDegree, capacity, limit);
+ MemoryPool queryPool =
MemoryManager.getInstance().addQueryMemoryPool();
Review Comment:
can we bind a MemoryPool for each graph
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedSchemaTransaction.java:
##########
@@ -204,14 +207,20 @@ private void invalidateCache(HugeType type, Id id) {
this.arrayCaches.remove(type, id);
}
+ /**
+ * Ids used in cache must be on-heap object
+ */
private static Id generateId(HugeType type, Id id) {
// NOTE: it's slower performance to use:
// String.format("%x-%s", type.code(), name)
- return IdGenerator.of(type.string() + "-" + id.asString());
+ return new IdGenerator.StringId(type.string() + "-" + id.asString());
Review Comment:
add a OnHeapIdGenerator.of() class?
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/consumer/factory/IdFactory.java:
##########
@@ -58,7 +58,9 @@ public BinaryBackendEntry.BinaryId newBinaryId(byte[] bytes,
Id id) {
.getCorrespondingTaskMemoryPool(
Thread.currentThread()
.getName());
- return new BinaryIdOffHeap(bytes, null,
+ return taskMemoryPool == null ?
Review Comment:
expect this.taskMemoryPool style
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/MemoryManager.java:
##########
@@ -89,6 +89,10 @@ private MemoryManager() {
}
public MemoryPool addQueryMemoryPool() {
+ if (MEMORY_MODE == MemoryMode.DISABLE_MEMORY_MANAGEMENT) {
+ return null;
+ }
+
int count = queryMemoryPools.size();
Review Comment:
expect this.xx stype
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/consumer/factory/IdFactory.java:
##########
@@ -58,7 +58,9 @@ public BinaryBackendEntry.BinaryId newBinaryId(byte[] bytes,
Id id) {
.getCorrespondingTaskMemoryPool(
Thread.currentThread()
.getName());
Review Comment:
prefer by Thread id instead of Thread name
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/traversers/KoutAPI.java:
##########
@@ -93,27 +97,40 @@ public String get(@Context GraphManager manager,
"'{}', max degree '{}', capacity '{}' and limit '{}'",
graph, source, direction, edgeLabel, depth,
nearest, maxDegree, capacity, limit);
+ MemoryPool queryPool =
MemoryManager.getInstance().addQueryMemoryPool();
+ Optional.ofNullable(queryPool).ifPresent(pool -> {
+ MemoryPool currentTaskPool = pool.addChildPool("kout-main-task");
+ MemoryManager.getInstance()
+
.bindCorrespondingTaskMemoryPool(Thread.currentThread().getName(),
+ (TaskMemoryPool)
currentTaskPool);
+ MemoryPool currentOperationPool =
currentTaskPool.addChildPool("kout-main-operation");
+ });
- ApiMeasurer measure = new ApiMeasurer();
+ try {
Review Comment:
prefer to add a wrapper method for MemoryPool init-and-gc, then call the
original method?
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/MemoryManager.java:
##########
@@ -89,6 +89,10 @@ private MemoryManager() {
}
public MemoryPool addQueryMemoryPool() {
+ if (MEMORY_MODE == MemoryMode.DISABLE_MEMORY_MANAGEMENT) {
+ return null;
+ }
+
int count = queryMemoryPools.size();
Review Comment:
What is the difference between QueryMemoryPool and MemoryPool? if no
difference, just naming MemoryPool is ok.
and can we make some special names for CorrespondingTaskMemoryPool and
CurrentWorkingOperatorMemoryPool, such as RequestMemoryPool and
RequestStageMemoryPool
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/cache/CachedGraphTransaction.java:
##########
@@ -260,6 +260,7 @@ private Iterator<HugeVertex> queryVerticesByIds(IdQuery
query) {
return QueryResults.emptyIterator();
}
if (needCacheVertex(vertex)) {
+ vertex.convertIdToOnHeapIfNeeded();
Review Comment:
it's ok to just call convert in HeapCache.update(), at the same time, we
avoid modifying the code everywhere
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/traversers/KoutAPI.java:
##########
@@ -93,27 +97,40 @@ public String get(@Context GraphManager manager,
"'{}', max degree '{}', capacity '{}' and limit '{}'",
graph, source, direction, edgeLabel, depth,
nearest, maxDegree, capacity, limit);
+ MemoryPool queryPool =
MemoryManager.getInstance().addQueryMemoryPool();
+ Optional.ofNullable(queryPool).ifPresent(pool -> {
+ MemoryPool currentTaskPool = pool.addChildPool("kout-main-task");
+ MemoryManager.getInstance()
+
.bindCorrespondingTaskMemoryPool(Thread.currentThread().getName(),
+ (TaskMemoryPool)
currentTaskPool);
+ MemoryPool currentOperationPool =
currentTaskPool.addChildPool("kout-main-operation");
+ });
Review Comment:
add a method like `graph.switchToMemoryPool("kout", "main")`?
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/MemoryManager.java:
##########
@@ -77,7 +77,7 @@ public class MemoryManager {
private final MemoryArbitrator memoryArbitrator;
private final ExecutorService arbitrateExecutor;
- private static MemoryMode MEMORY_MODE =
MemoryMode.ENABLE_OFF_HEAP_MANAGEMENT;
+ private static MemoryMode MEMORY_MODE =
MemoryMode.DISABLE_MEMORY_MANAGEMENT;
Review Comment:
MEMORY_MODE style is only for const var, and if there is only a single
MemoryManager, also remove static mark: `private MemoryMode memoryMode`
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/memory/consumer/factory/IdFactory.java:
##########
@@ -58,7 +58,9 @@ public BinaryBackendEntry.BinaryId newBinaryId(byte[] bytes,
Id id) {
.getCorrespondingTaskMemoryPool(
Thread.currentThread()
.getName());
- return new BinaryIdOffHeap(bytes, null,
+ return taskMemoryPool == null ?
+ new BinaryBackendEntry.BinaryId(bytes, id) :
+ new BinaryIdOffHeap(bytes, null,
Review Comment:
We can directly get the memory pool at one time
`MemoryManager.getInstance().currentMemoryPool()`, it does the 2 steps:
getCorrespondingTaskMemoryPool, getCurrentWorkingOperatorMemoryPool.
--
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]
