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

Performance issue on vertex cache with Guava implementation #3185

Closed
ministat opened this issue Aug 27, 2022 · 4 comments
Closed

Performance issue on vertex cache with Guava implementation #3185

ministat opened this issue Aug 27, 2022 · 4 comments

Comments

@ministat
Copy link
Contributor

JanusGraph's GuavaVertexCache will cause memory leak in 100% read and heavy read scenario, especially used in OLAP cases. The Guava cache should be replaced by Caffiene cache to fix this issue according to Guava's community feedback.
See google/guava#2408 and google/guava#2063.

In my local JanusGraph, after I replaced Guava cache with Caffeine cache, the OLAP case, for example, g.V().count() takes 18 minutes to finish the scan of a graph with 500 million vertex and 6 billion edges. Before this fix, it takes 45 minutes. So, the fix gets ~3X benefit.

For confirmed bugs, please report:

Stack Trace (if you have one)

paste stack trace here
@li-boxuan li-boxuan changed the title Performance bug on vertex cache with Guava implementation Performance issue on vertex cache with Guava implementation Aug 27, 2022
@li-boxuan
Copy link
Member

Thanks for reporting @ministat! Seems a duplicate of #871 (which was created by the author of Guava, interesting!). Usually, we close newer issues if they are duplicates, but your discovery and experiments are very valuable so I'd like to keep both issues open. Would you like to contribute your implementation back to the community by submitting a PR? That would be very helpful!

@ministat
Copy link
Contributor Author

ministat commented Aug 28, 2022 via email

@porunov porunov added this to the Release v1.0.0 milestone Aug 29, 2022
@porunov
Copy link
Member

porunov commented Aug 29, 2022

Identified 8 Guava caches in JanusGraph. Posted them here: #871 (comment)

ministat pushed a commit to ministat/janusgraph that referenced this issue Aug 29, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <hongjiang.zhang@gmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 29, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 30, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 30, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 30, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 31, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
ministat added a commit to ministat/janusgraph that referenced this issue Aug 31, 2022
Fix the issue JanusGraph#3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
porunov pushed a commit that referenced this issue Aug 31, 2022
Fix the issue #3185

Signed-off-by: Hongjiang Zhang <timerzhj@hotmail.com>
@porunov
Copy link
Member

porunov commented Aug 31, 2022

Fixed in #3188
The continuation of work to replace other 7 Guava caches will be tracked in the next issue: #871

@porunov porunov closed this as completed Aug 31, 2022
porunov added a commit to porunov/janusgraph that referenced this issue Aug 31, 2022
This is a follow-up commit to commit 9a22a1a

Move the rest (7) caches from Guava implementation to Caffeine implementation

This commit also removes a previously added Benchmark class with the obsolate GuavaVertexCache.class. The benchmark is provided in the next GitHub PR: JanusGraph#3188 which shows that Caffeine has better performance characteristics in all scenarious.

ConcurencyLevel is not provided for cache implementation. The reason for this can be found by the following link https://groups.google.com/g/guava-discuss/c/BnoN9q9fDgw/m/6kF6H2aLAgAJ

Related to JanusGraph#3188 and JanusGraph#3185

Partially related to JanusGraph#2033

Fixes JanusGraph#871
porunov added a commit to porunov/janusgraph that referenced this issue Aug 31, 2022
This is a follow-up commit to commit 9a22a1a

Move the rest (7) caches from Guava implementation to Caffeine implementation

This commit also removes a previously added Benchmark class with the obsolate GuavaVertexCache.class. The benchmark is provided in the next GitHub PR: JanusGraph#3188 which shows that Caffeine has better performance characteristics in all scenarious.

ConcurencyLevel is not provided for cache implementation. The reason for this can be found by the following link https://groups.google.com/g/guava-discuss/c/BnoN9q9fDgw/m/6kF6H2aLAgAJ

Related to JanusGraph#3188 and JanusGraph#3185

Partially related to JanusGraph#2033

Fixes JanusGraph#871

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit to porunov/janusgraph that referenced this issue Aug 31, 2022
This is a follow-up commit to the 9a22a1a commit

Migrates the rest 7 caches from Guava implementation to Caffeine implementation

This commit also removes a previously added Benchmark class with the obsolate GuavaVertexCache.class. The benchmark is provided in the next GitHub PR: JanusGraph#3188 which shows that Caffeine has better performance characteristics in all scenarious.

ConcurencyLevel is not provided for the cache implementation anymore. The reason for this can be found by the following link https://groups.google.com/g/guava-discuss/c/BnoN9q9fDgw/m/6kF6H2aLAgAJ

Related to JanusGraph#3188 and JanusGraph#3185

Partially related to JanusGraph#2033

Fixes JanusGraph#871

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit that referenced this issue Sep 1, 2022
This is a follow-up commit to the 9a22a1a commit

Migrates the rest 7 caches from Guava implementation to Caffeine implementation

This commit also removes a previously added Benchmark class with the obsolate GuavaVertexCache.class. The benchmark is provided in the next GitHub PR: #3188 which shows that Caffeine has better performance characteristics in all scenarious.

ConcurencyLevel is not provided for the cache implementation anymore. The reason for this can be found by the following link https://groups.google.com/g/guava-discuss/c/BnoN9q9fDgw/m/6kF6H2aLAgAJ

Related to #3188 and #3185

Partially related to #2033

Fixes #871

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
porunov added a commit that referenced this issue Sep 22, 2022
This is a follow-up commit to the 9a22a1a commit

Migrates the rest 7 caches from Guava implementation to Caffeine implementation

This commit also removes a previously added Benchmark class with the obsolate GuavaVertexCache.class. The benchmark is provided in the next GitHub PR: #3188 which shows that Caffeine has better performance characteristics in all scenarious.

ConcurencyLevel is not provided for the cache implementation anymore. The reason for this can be found by the following link https://groups.google.com/g/guava-discuss/c/BnoN9q9fDgw/m/6kF6H2aLAgAJ

Related to #3188 and #3185

Partially related to #2033

Fixes #871

Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants