Performance Optimization: Lazy Load Vertex Relations #4343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntisseyre
force-pushed
the
lazy_relations
branch
3 times, most recently
from
821111c to
dfbb92e
Compare
ntisseyre
force-pushed
the
lazy_relations
branch
4 times, most recently
from
8a8c0d2 to
1711b2b
Compare
porunov
approved these changes
Apr 9, 2024
There was a problem hiding this comment.
Thank you @ntisseyre ! LGTM. I have left two nitpicks, but otherwise it looks good.
@JanusGraph/committers merging this PR using lazy consensus in a week.
janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphLazyRelation.java
Outdated
Show resolved
Hide resolved
janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/TransactionConfiguration.java
Show resolved
Hide resolved
porunov
reviewed
Apr 9, 2024
ntisseyre
force-pushed
the
lazy_relations
branch
2 times, most recently
from
bb98134 to
367df51
Compare
li-boxuan
reviewed
Apr 10, 2024
There was a problem hiding this comment.
I love the idea of lazy-loading but I am not sure if I understand when this is useful and how powerful it is. Is it possible to add a new benchmark under janusgraph-benchmark module?
Could you also please run a full build with your feature turned on by default? You could create a bogus PR to trigger CI runs. I would love to see if that can help us capture bugs.
|
|
||
| import java.util.Iterator; | ||
|
|
||
| public class JanusGraphLazyProperty<V> extends JanusGraphLazyRelation<V> implements JanusGraphVertexProperty<V> { |
There was a problem hiding this comment.
Consider: renaming this to JanusGraphLazyVertexProperty. A "property" can be attached to a vertex, an edge, or a vertex property.
| * <p> | ||
| * When enabled, it can have a boost on large scale read operations, when only certain type of relations are being read. | ||
| * | ||
| * @return Object with the skip db-cache reads check settings |
| @@ -320,6 +326,7 @@ public void close() { | |||
| config.getVertexCacheSize(), effectiveVertexCacheSize, MIN_VERTEX_CACHE_SIZE); | |||
| } | |||
|
|
|||
| relTypeCache = new ConcurrentHashMap<>(30); | |||
There was a problem hiding this comment.
If only used by JanusGraphLazyRelation, looks like there's no reason to initialize relTypeCache if this transaction doesn't have lazyLoadRelations() enabled?
janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphLazyRelation.java
Show resolved
Hide resolved
ntisseyre
force-pushed
the
lazy_relations
branch
6 times, most recently
from
9709a6f to
d725e10
Compare
Signed-off-by: ntisseyre <ntisseyre@apple.com>
ntisseyre
force-pushed
the
lazy_relations
branch
from
d725e10 to
cd52b37
Compare
|
Comments explained in #4367 (comment) make sense to me. @li-boxuan will you be able to check if you have any concerns or other comments regarding this PR? |
li-boxuan
approved these changes
Apr 24, 2024
Thank you @li-boxuan ! |
You should just add a new param into you test and annotate it. The benchmark test will detect all the annotated parameters and will execute tests with all combinations of all provided values. Here is an example of a boolean parameter (can be seen in this benchmark test): You can create a similar param for lazy-loading feature. A quick thing you can do in the PR #4367 :
You can see the following report under that CI job: Some things to notice:
For reference: |
Thank you @porunov ! |
It was not triggered due to failed checkstyle: |
Oh, thanks! Fixed |
Hmm. Seems OOM. So, it seems never executed any benchmark test with |
I have reduced it to only 5k vertices and put isLazyLoad=true to be executed first. |
|
Below are the tests I executed on my local laptop with minimal parallel processes running. Master branch basic benchmark tests: Current PR basic benchmark tests: The benchmark test introduced in #4367 Conclusion: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Optimizing reads of large amounts of vertexes by doing lazy-load (lazy-deserialization) of vertex properties or edges.
Lazy-load is not enabled by default and should be set explicitly on a
TransactionBuilder.Leveraging vertex's query-cache for multiQuery operations instead of rebuilding the query while loading vertex relations.