Skip to content

Conversation

@Pengzna
Copy link
Collaborator

@Pengzna Pengzna commented Dec 9, 2024

wip...

@codecov
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 34.35115% with 86 lines in your changes missing coverage. Please review.

Project coverage is 37.56%. Comparing base (f0b1395) to head (db3ee80).
Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
...e/hugegraph/memory/consumer/factory/IdFactory.java 0.00% 39 Missing ⚠️
...a/org/apache/hugegraph/api/traversers/KoutAPI.java 61.90% 7 Missing and 1 partial ⚠️
...graph/memory/consumer/factory/PropertyFactory.java 0.00% 8 Missing ⚠️
...g/apache/hugegraph/backend/store/BackendTable.java 0.00% 7 Missing ⚠️
...main/java/org/apache/hugegraph/util/Consumers.java 57.14% 5 Missing and 1 partial ⚠️
...graph/backend/cache/CachedSchemaTransactionV2.java 0.00% 5 Missing ⚠️
...g/apache/hugegraph/structure/HugeEdgeProperty.java 0.00% 3 Missing ⚠️
...ava/org/apache/hugegraph/schema/SchemaElement.java 33.33% 1 Missing and 1 partial ⚠️
.../java/org/apache/hugegraph/structure/HugeEdge.java 66.66% 1 Missing and 1 partial ⚠️
...ava/org/apache/hugegraph/structure/HugeVertex.java 66.66% 1 Missing and 1 partial ⚠️
... and 4 more

❗ There is a different number of reports uploaded between BASE (f0b1395) and HEAD (db3ee80). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (f0b1395) HEAD (db3ee80)
7 4
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2704      +/-   ##
============================================
- Coverage     46.99%   37.56%   -9.43%     
+ Complexity      821      588     -233     
============================================
  Files           745      756      +11     
  Lines         60062    62108    +2046     
  Branches       7670     8068     +398     
============================================
- Hits          28225    23330    -4895     
- Misses        29014    36266    +7252     
+ Partials       2823     2512     -311     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

"'{}', max degree '{}', capacity '{}' and limit '{}'",
graph, source, direction, edgeLabel, depth,
nearest, maxDegree, capacity, limit);
MemoryPool queryPool = MemoryManager.getInstance().addQueryMemoryPool();
Copy link
Contributor

@javeme javeme Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we bind a MemoryPool for each graph, or bind MemoryManager to graph?

.bindCorrespondingTaskMemoryPool(Thread.currentThread().getName(),
(TaskMemoryPool) currentTaskPool);
MemoryPool currentOperationPool = currentTaskPool.addChildPool("kout-main-operation");
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a method like graph.switchToMemoryPool("kout", "main")?

});

ApiMeasurer measure = new ApiMeasurer();
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to add a wrapper method for MemoryPool init-and-gc, then call the original method?

return QueryResults.emptyIterator();
}
if (needCacheVertex(vertex)) {
vertex.convertIdToOnHeapIfNeeded();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok to just call convert in HeapCache.update(), at the same time, we avoid modifying the code everywhere

// 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a OnHeapIdGenerator.of() class?

Thread.currentThread()
.getName());
return new BinaryIdOffHeap(bytes, null,
return taskMemoryPool == null ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect this.taskMemoryPool style

TaskMemoryPool taskMemoryPool = (TaskMemoryPool) MemoryManager.getInstance()
.getCorrespondingTaskMemoryPool(
Thread.currentThread()
.getName());
Copy link
Contributor

@javeme javeme Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer by Thread id instead of Thread name to avoid duplicate names

return null;
}

int count = queryMemoryPools.size();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect this.xx stype

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private final ExecutorService arbitrateExecutor;

private static MemoryMode MEMORY_MODE = MemoryMode.ENABLE_OFF_HEAP_MANAGEMENT;
private static MemoryMode MEMORY_MODE = MemoryMode.DISABLE_MEMORY_MANAGEMENT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEMORY_MODE style is only for const var, and if there is only a single MemoryManager, also remove static mark: private MemoryMode memoryMode

return new BinaryIdOffHeap(bytes, null,
return taskMemoryPool == null ?
new BinaryBackendEntry.BinaryId(bytes, id) :
new BinaryIdOffHeap(bytes, null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can directly get the memory pool at one time MemoryManager.getInstance().currentMemoryPool(), it does the 2 steps:
getCorrespondingTaskMemoryPool, getCurrentWorkingOperatorMemoryPool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and can we rename some methods for a more concise and consistent style:

  1. getCorrespondingTaskMemoryPool => currentTaskMemoryPool
  2. getCurrentWorkingOperatorMemoryPool => currentOperatorMemoryPool
  3. MemoryPool.getSnapShot() => MemoryPool.getStats()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also move MemoryPoolStats out from impl?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to get MemoryPool from threadlocal

@github-actions
Copy link

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement to our memory performance, looking forward to merging it soon.

.bindCorrespondingTaskMemoryPool(Thread.currentThread().getName(),
(TaskMemoryPool) currentTaskPool);
MemoryPool currentOperationPool =
currentTaskPool.addChildPool("kout-consume-operation");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Consumers is a general class, seems it's not appropriate to put the upper-level kout code here

this.runningFutures.add(
this.executor.submit(new ContextCallable<>(this::runAndDone)));
this.executor.submit(
new ContextCallable<>(() -> this.runAndDone(MemoryManager.getInstance()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this TaskMemoryPool-bind code into ContextCallable?

return new BinaryIdOffHeap(bytes, null,
return taskMemoryPool == null ?
new BinaryBackendEntry.BinaryId(bytes, id) :
new BinaryIdOffHeap(bytes, null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and can we rename some methods for a more concise and consistent style:

  1. getCorrespondingTaskMemoryPool => currentTaskMemoryPool
  2. getCurrentWorkingOperatorMemoryPool => currentOperatorMemoryPool
  3. MemoryPool.getSnapShot() => MemoryPool.getStats()

MemoryManager.getInstance()
.bindCorrespondingTaskMemoryPool(Thread.currentThread().getName(),
(TaskMemoryPool) currentTaskPool);
MemoryPool currentOperationPool = currentTaskPool.addChildPool("kout-main-operation");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can let:

  1. MemoryPool.addChildPool return QueryMemoryPool
  2. QueryMemoryPool.addChildPool return TaskMemoryPool
  3. TaskMemoryPooll.addChildPool return OperatorMemoryPool

return new BinaryIdOffHeap(bytes, null,
return taskMemoryPool == null ?
new BinaryBackendEntry.BinaryId(bytes, id) :
new BinaryIdOffHeap(bytes, null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also move MemoryPoolStats out from impl?

}
return manager.serializer(g, measure.measures()).writeList("vertices", ids);
} finally {
Optional.ofNullable(queryPool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer if-style

return new BinaryIdOffHeap(bytes, null,
return taskMemoryPool == null ?
new BinaryBackendEntry.BinaryId(bytes, id) :
new BinaryIdOffHeap(bytes, null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to get MemoryPool from threadlocal

this.isOutEdge = true;
}

public void convertIdToOnHeapIfNeeded() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to super class?

return new HugeEdgeProperty<>(this, pkey, val);
Class<V> valueType = (Class<V>) val.getClass();
PropertyFactory<V> propertyFactory = PropertyFactory.getInstance(valueType);
return propertyFactory.newHugeEdgeProperty(this, pkey, val);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have no OffheapEdge?

new HugeVertexProperty<>(owner, key, value) :
new HugeVertexPropertyOffHeap<>(
taskMemoryPool.getCurrentWorkingOperatorMemoryPool(), owner, key,
value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid bytes copy in serializeSelfToByteBuf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. preter to let OffHeapObject.serializeSelfToByteBuf return ByteBuf.
  2. can we rename OffHeapObject.zeroCopyReadFromByteBuf to readAsHeapObject?
  3. OffHeapObject.getAllMemoryBlock is useless? will we release an object individually?
  4. HugeVertexPropertyOffHeap.isPresent optimize with 'valueOffHeap != null'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants