Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): support MemoryManagement for graph query framework #2649

Merged
merged 51 commits into from
Nov 5, 2024

Conversation

Pengzna
Copy link
Contributor

@Pengzna Pengzna commented Aug 25, 2024

As title.

Users can use following new options to configure memory management:

# The memory management switch in HugeGraph. Options: off-heap, on-heap, disable.
memory.mode=off-heap
# The maximum memory capacity that can be managed for all queries in HugeGraph.
memory.max_capacity=1073741824
# The maximum memory capacity that can be managed for a query in HugeGraph.
memory.one_query_max_capacity=104857600
# The alignment used for round memory size.
memory.alignment=8

Detailed docs:

image

Copy link

codecov bot commented Aug 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 1046 lines in your changes missing coverage. Please review.

Project coverage is 1.19%. Comparing base (21855c6) to head (b77346b).

Files with missing lines Patch % Lines
...e/hugegraph/memory/consumer/factory/IdFactory.java 0.00% 102 Missing ⚠️
...ache/hugegraph/memory/pool/AbstractMemoryPool.java 0.00% 99 Missing ⚠️
...gegraph/memory/consumer/impl/id/EdgeIdOffHeap.java 0.00% 94 Missing ⚠️
...hugegraph/memory/pool/impl/OperatorMemoryPool.java 0.00% 93 Missing ⚠️
...ava/org/apache/hugegraph/memory/MemoryManager.java 0.00% 88 Missing ⚠️
...he/hugegraph/memory/pool/impl/QueryMemoryPool.java 0.00% 70 Missing ⚠️
...gegraph/memory/consumer/impl/id/UuidIdOffHeap.java 0.00% 47 Missing ⚠️
...he/hugegraph/memory/pool/impl/MemoryPoolStats.java 0.00% 47 Missing ⚠️
...che/hugegraph/memory/pool/impl/TaskMemoryPool.java 0.00% 38 Missing ⚠️
...gegraph/memory/consumer/impl/id/LongIdOffHeap.java 0.00% 37 Missing ⚠️
... and 19 more

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

HEAD has 6 uploads less than BASE
Flag BASE (21855c6) HEAD (b77346b)
7 1
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #2649       +/-   ##
============================================
- Coverage     47.70%   1.19%   -46.52%     
+ Complexity      821       8      -813     
============================================
  Files           720     723        +3     
  Lines         58989   57517     -1472     
  Branches       7607    7214      -393     
============================================
- Hits          28143     685    -27458     
- Misses        28031   56781    +28750     
+ Partials       2815      51     -2764     

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

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.

How to re-point a Java variable to an off-heap object? we can set its address to an off-heap memory:

But! There is a risk in using off-heap objects, that is, off-heap objects refer to on-heap objects, such as vertex label, which may be released by schema cache.
We have two ways to solve this risk:

  1. One way is to pin the referenced in-heap object when an off-heap object is created, such as saving it in a hashset collection;
  2. The other way is to refresh the in-heap object when taking an off-heap object (such as re-get it from the schema cache by label id).

@Pengzna Pengzna marked this pull request as ready for review October 29, 2024 07:13
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. feature New feature labels Oct 29, 2024
@Pengzna
Copy link
Contributor Author

Pengzna commented Oct 29, 2024

@javeme @imbajin @zyxxoo @MingzhenHan PTAL~

imbajin
imbajin previously approved these changes Nov 3, 2024
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

Thanks & merge it first~

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 3, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this file?

Copy link
Member

Choose a reason for hiding this comment

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

why change this file?

maybe influenced by Fury dependencies🤔?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why change this file?

maybe influenced by Fury dependencies🤔?

Yes, and I ran wrong regenerate_known_dependencies.sh which is in hugegraph-dist/scripts/dependency, resulting in a duplicate known-dependencies.txt under hugegraph-dist/scripts/dependency.

Actually, only regenerate_known_dependencies.sh in /install-dist/scripts/dependency is enough and correct. regenerate_known_dependencies.sh in hugegraph-dist/scripts/dependency seems useless and may cause misunderstanding for programmers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

@imbajin maybe could entirely remove hugegraph-commons/hugegraph-dist?

@imbajin imbajin changed the title feat(server): Introduce MemoryManagement for HugeGraph Query Framework feat(server): support MemoryManagement for graph query framework Nov 5, 2024
@imbajin imbajin merged commit 35e5a8c into apache:master Nov 5, 2024
16 of 17 checks passed

public class FurySerializationUtil {

public static final Fury FURY = Fury.builder().withLanguage(Language.JAVA)
Copy link

@chaokunyang chaokunyang Nov 8, 2024

Choose a reason for hiding this comment

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

Seems Fury is used to serialize/deserialize object to/from offheap, serialized payload size may be not important here. Maybe we can disable number compress here by FuryBuilder#withNumerCompressed(false). If we have lots of number for serialization, disable this option will have much better performance

Choose a reason for hiding this comment

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

This Fury is not thread safe, please use ThreadSafeFury instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestions! All these will be improved in next pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants