janusgraph icon indicating copy to clipboard operation
janusgraph copied to clipboard

Disable vertex cache for cardinality checks

Open cdegroc opened this issue 1 year ago • 1 comments

In a service running JanusGraph embedded, I observed a number of sizeable allocations coming from JanusGraphVertexFeatures#getCardinality.

From a profile (included below), it looks like a StandardJanusGraphTx transaction is created when checking a vertex property's cardinality. The transaction itself allocates a CaffeineVertexCache. The CaffeineVertexCache internally creates a NonBlockingHashMap which initial size is configured in the GraphDatabaseConfiguration class or the graph's dynamic configuration.

I think we don't need a vertex cache for cardinality checks and would suggest disabling it to release some memory.

Screenshot 2024-06-11 at 10 20 37


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:

For all changes:

  • [ ] Is there an issue associated with this PR? Is it referenced in the commit message?
  • [ ] Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
  • [ ] Is your initial contribution a single, squashed commit?

For code changes:

  • [ ] Have you written and/or updated unit tests to verify your changes?
  • [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • [ ] If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • [ ] If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • [ ] Have you ensured that format looks appropriate for the output in which it is rendered?

cdegroc avatar Jun 11 '24 08:06 cdegroc

Great finding, thank you!

btw why is this PR marked as WIP?

I wanted to confirm the problem before creating an issue and a proper PR. I'm fine with merging as-is if that's good enough.

cdegroc avatar Jun 26 '24 08:06 cdegroc

I usually have a default vertex cache size and dirty vertex size set to 0 and set a proper value per transaction when needed, thus I haven't bumped into this issue before. However, the solution here perfectly makes sense to me as the transaction is closed immediately after we exiting this method. We don't create any new mutations, thus dirty vertex size should be 0 and read-only transaction make sense. We return a single property key, and don't reuse this transaction for anything else, thus vertex cache size 0 makes sense here. LGTM. Thank you @cdegroc !

Thank you so much for confirming @porunov 🙇

cdegroc avatar Aug 19 '24 07:08 cdegroc