Release memory where possible in RestElasticSearchClient #4685

criminosis · 2024-09-26T20:41:29Z

Closes #4684

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?

criminosis · 2024-09-26T20:42:46Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java

+            serializedSize += 1; //For follow-up NEW_LINE_BYTES
            if (this.requestSource != null) {
                serializedSize += this.requestSource.length;
-                serializedSize+= 1; //For follow-up NEW_LINE_BYTES
+                serializedSize += 1; //For follow-up NEW_LINE_BYTES


Fixed some formatting that I missed previously

criminosis · 2024-09-26T20:43:14Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java

+            ListIterator<ElasticSearchMutation> requestsIter = requests.listIterator();
+            while (requestsIter.hasNext()) {
+                ElasticSearchMutation request = requestsIter.next();
+                //Remove the element from the collection so the collection's reference to it doesn't hold it from being
+                //GC'ed after it has been converted to its serialized form
+                requestsIter.set(null);


Addressing point 1 in #4684

This is mutating the List that is passed in, which will then be exposed to the caller since we can't take ownership of the List in Java in this context. However from reviewing the call sites, they create the list within their function scopes, and don't do anything with it after passing it into here so this should be "safe", but mutating a passed in list is sometimes frowned upon in Java, so I wanted to call that out.

Call Hierarchy analysis by IntelliJ finds:

This constructor is only called here

bulkRequest is only called here, here, here, and here

In each case the passed in List is created in that callsite and isn't mutated, or even read, after being passed to here, at least at the time of this writing.

criminosis · 2024-09-26T20:44:00Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java

+                RequestBytes next = requestIterator.next();
+                //Remove the element from the collection, so the iterator doesn't prevent it from being GC'ed
+                //due to the reference to it in the collection
+                requestIterator.set(null);


Addressing point 2 in #4684

criminosis · 2024-09-26T20:45:14Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java

@@ -490,14 +500,20 @@ class BulkRequestChunker implements Iterator<List<RequestBytes>> {
        //There is no "correct" number of actions to perform in a single bulk request. Experiment with different
        // settings to find the optimal size for your particular workload. Note that Elasticsearch limits the maximum
        // size of a HTTP request to 100mb by default
-        private final PeekingIterator<RequestBytes> requestIterator;
+        private final ListIterator<RequestBytes> requestIterator;


Had to expose being able to set the element to null, which isn't exposed in PeekingIterator so took on the peek() behavior here, and switched to ListIterator

criminosis · 2024-09-26T20:46:00Z

janusgraph-es/src/main/java/org/janusgraph/diskstorage/es/rest/RestElasticSearchClient.java

+        private int writeTo(byte[] target, int initialOffset) {
+            int offset = initialOffset;
+            System.arraycopy(this.requestBytes, 0, target, offset, this.requestBytes.length);
+            offset += this.requestBytes.length;
+            System.arraycopy(NEW_LINE_BYTES, 0, target, offset, NEW_LINE_BYTES.length);
+            offset += NEW_LINE_BYTES.length;
            if (this.requestSource != null) {
-                outputStream.write(requestSource);
-                outputStream.write(NEW_LINE_BYTES);
+                System.arraycopy(this.requestSource, 0, target, offset, this.requestSource.length);
+                offset += this.requestSource.length;
+                System.arraycopy(NEW_LINE_BYTES, 0, target, offset, NEW_LINE_BYTES.length);
+                offset += NEW_LINE_BYTES.length;
            }
+            return offset;
        }
    }

-    private Pair<String, byte[]> buildBulkRequestInput(List<RequestBytes> requests, String ingestPipeline) throws IOException {
-        final ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+    private Pair<String, byte[]> buildBulkRequestInput(List<RequestBytes> requests, String ingestPipeline) {
+        int totalBytes = requests.stream().mapToInt(RequestBytes::getSerializedSize).sum();
+        //By making a singular array we copy into avoids any dynamically expanded growth of the array that may overshoot
+        //how much memory we actually need, additionally it also avoids a final copy at the end normally done by
+        //ByteArrayOutputStream's toByteArray()
+        byte[] bytes = new byte[totalBytes];
+        int offset = 0;
        for (final RequestBytes request : requests) {
-            request.writeTo(outputStream);
+            //We can't remove the element from the collection like we do elsewhere, because we need to retain the
+            //serialized form in case of an error so the error can be paired to the originating request based on index
+            offset = request.writeTo(bytes, offset);


Addressing point 3 in #4684

criminosis · 2024-09-26T21:09:34Z

IIRC @porunov & @li-boxuan you two reviewed this code last time, so this may interest you as an enhancement on that submission. As always happy to make changes, hoping this may also help others that bump into OOMs in this area 😅 .

porunov · 2024-09-27T10:57:17Z

Hi @criminosis !
Thank you so much for this optimization. However, I want to notice that currently ElasticSearch tests don't pass for this PR.
The failures are logged below:

Error:    RestClientBulkRequestsTest.testThrowingIfSingleBulkItemIsLargerThanLimit:154 Should have thrown due to bulk request item being too large ==> Unexpected exception type thrown, expected: <java.lang.IllegalArgumentException> but was: <java.lang.UnsupportedOperationException>
Error:    RestClientRetryTest.testNonRetryErrorCodeException:202 expected: <responseException> but was: <java.lang.UnsupportedOperationException>
Error:    RestClientRetryTest.testRetriesExhaustedReturnsLastRetryException:181 expected: <responseException> but was: <java.lang.UnsupportedOperationException>
Error:    RestClientRetryTest.testRetryOnConfiguredErrorStatus:151 expected: <java.io.IOException: Expected> but was: <java.lang.UnsupportedOperationException>

Closes JanusGraph#4684 Signed-off-by: Allan Clements <[email protected]>

criminosis · 2024-09-27T12:05:47Z

Thanks @porunov . Unfortunately missed those from a larger failure with the index provider tests I fixed before turning in last night. Sorry about that 🤦‍♂️ .

Those tests were passing in an immutable list which blows up at runtime when the iterator attempts to call .set(null) on a particular index. I changed them to be passing in a mutable list and validated they passed locally since my laptop could easily handle these unit tests 😅 .

criminosis · 2024-10-02T23:16:08Z

I'm going to pull this back to draft status. I think I have discovered a behavior asymmetry between the Java reference driver and the Rust (unofficial) driver. It seems like sessions aren't getting closed in JanusGraph in the internal Tinkerpop server and the Out Of Memory Errors that prompted this PR are just the part of the iceberg that stuck out of the water.

criminosis force-pushed the memory_pressure branch from 24c3181 to 6703d18 Compare September 26, 2024 20:42

criminosis commented Sep 26, 2024

View reviewed changes

criminosis force-pushed the memory_pressure branch from 6703d18 to 66ccdc8 Compare September 27, 2024 03:47

Release memory where possible in RestElasticSearchClient

75d6f51

Closes JanusGraph#4684 Signed-off-by: Allan Clements <[email protected]>

criminosis force-pushed the memory_pressure branch from 66ccdc8 to 75d6f51 Compare September 27, 2024 11:58

criminosis marked this pull request as draft October 2, 2024 23:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release memory where possible in RestElasticSearchClient #4685

Release memory where possible in RestElasticSearchClient #4685

criminosis commented Sep 26, 2024 •

edited

Loading

criminosis Sep 26, 2024

criminosis Sep 26, 2024

criminosis Sep 26, 2024 •

edited

Loading

criminosis Sep 26, 2024 •

edited

Loading

criminosis Sep 26, 2024

criminosis Sep 26, 2024

criminosis commented Sep 26, 2024

porunov commented Sep 27, 2024

criminosis commented Sep 27, 2024

criminosis commented Oct 2, 2024

Release memory where possible in RestElasticSearchClient #4685

Are you sure you want to change the base?

Release memory where possible in RestElasticSearchClient #4685

Conversation

criminosis commented Sep 26, 2024 • edited Loading

For all changes:

For code changes:

For documentation related changes:

criminosis Sep 26, 2024

Choose a reason for hiding this comment

criminosis Sep 26, 2024

Choose a reason for hiding this comment

criminosis Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

criminosis Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

criminosis Sep 26, 2024

Choose a reason for hiding this comment

criminosis Sep 26, 2024

Choose a reason for hiding this comment

criminosis commented Sep 26, 2024

porunov commented Sep 27, 2024

criminosis commented Sep 27, 2024

criminosis commented Oct 2, 2024

criminosis commented Sep 26, 2024 •

edited

Loading

criminosis Sep 26, 2024 •

edited

Loading

criminosis Sep 26, 2024 •

edited

Loading