Skip to content

Releasing child request builder memory from BulkRequestBuilder #105194

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.

Sign up for GitHub

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

Merged
masseyke merged 6 commits into from
Feb 7, 2024
+37 −9
Merged

Releasing child request builder memory from BulkRequestBuilder #105194

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cdc4dd4
Releasing request memory from BulkRequestBuilder
masseyke Feb 6, 2024
3613875
Adding a couple of test assertions
masseyke Feb 6, 2024
80e8952
Adding a runtime assertion
masseyke Feb 6, 2024
40a280c
using an ArrayDeque for better runtime performance
masseyke Feb 7, 2024
bbacde4
Merge branch 'main' into release-bulk-request-builder-memory
masseyke Feb 7, 2024
289152d
simplifying for loop
masseyke Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions server/src/main/java/org/elasticsearch/action/bulk/BulkRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

/**
Expand All @@ -51,6 +52,7 @@ public class BulkRequestBuilder extends ActionRequestLazyBuilder<BulkRequest, Bu
private String globalPipeline;
private String globalRouting;
private WriteRequest.RefreshPolicy refreshPolicy;
private boolean requestPreviouslyCalled = false;

public BulkRequestBuilder(ElasticsearchClient client, @Nullable String globalIndex) {
super(client, BulkAction.INSTANCE);
Expand Down Expand Up @@ -199,11 +201,18 @@ public BulkRequestBuilder setRefreshPolicy(String refreshPolicy) {

@Override
public BulkRequest request() {
assert requestPreviouslyCalled == false : "Cannot call request() multiple times on the same BulkRequestBuilder object";
if (requestPreviouslyCalled) {
throw new IllegalStateException("Cannot call request() multiple times on the same BulkRequestBuilder object");
}
requestPreviouslyCalled = true;
validate();
BulkRequest request = new BulkRequest(globalIndex);
for (ActionRequestLazyBuilder<? extends DocWriteRequest<?>, ? extends DocWriteResponse> requestBuilder : requestBuilders) {
DocWriteRequest<?> childRequest = requestBuilder.request();
for (Iterator<ActionRequestLazyBuilder<? extends DocWriteRequest<?>, ? extends DocWriteResponse>> requestsIter = requestBuilders
.iterator(); requestsIter.hasNext();) {
DocWriteRequest<?> childRequest = requestsIter.next().request();
request.add(childRequest);
requestsIter.remove(); // The inner request builder can now be garbage collected

@DaveCTurner DaveCTurner Feb 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe removing items one-by-one from an ArrayList iterator is an O(N²) operation because each .remove() call has to shuffle all the remaining elements down by one. Maybe just set them to null instead and then clear the list at the end? Or maybe use a queue instead?

@DaveCTurner DaveCTurner Feb 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or maybe use a queue instead?

Yeah ArrayDeque is essentially an ArrayList with a moveable head so it'd work better here.

@arteam arteam Feb 6, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can avoid explicitly using the iterator and just clear the ArrayList after iterating over it.

for (var requestBuilder : requestBuilders) {
    request.add(requestBuilder.request());
}
requestBuilders.clear();

@DaveCTurner DaveCTurner Feb 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Waiting until the end and then clearing all the builders still has the 2x peak memory usage problem that we're trying to solve.

@masseyke masseyke Feb 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. I'll rework this. According to the ArrayDeque javadocs

Most ArrayDeque operations run in amortized constant time. Exceptions include remove, removeFirstOccurrence, removeLastOccurrence, contains, iterator.remove(), and the bulk operations, all of which run in linear time.

So it might not be the way to go either. Maybe null and clear (or new list -- seems like i've seen clear be slow for large lists) is the way to go.

@DaveCTurner DaveCTurner Feb 7, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But you wouldn't need to use any of those O(N) operations with ArrayDeque, you'd be calling pollFirst() which is definitely O(1).

@masseyke masseyke Feb 7, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, right. :)

}
for (DocWriteRequest<?> childRequest : requests) {
request.add(childRequest);
Expand Down
16 changes: 16 additions & 0 deletions server/src/test/java/org/elasticsearch/action/bulk/BulkRequestBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.test.ESTestCase;

import static org.hamcrest.Matchers.equalTo;

public class BulkRequestBuilderTests extends ESTestCase {

public void testValidation() {
Expand All @@ -20,4 +22,18 @@ public void testValidation() {
bulkRequestBuilder.add(new IndexRequest());
expectThrows(IllegalStateException.class, bulkRequestBuilder::request);
}

public void testRequestTwice() {
BulkRequestBuilder bulkRequestBuilder = new BulkRequestBuilder(null, null);
bulkRequestBuilder.add(new IndexRequestBuilder(null, randomAlphaOfLength(10)));
bulkRequestBuilder.add(new IndexRequestBuilder(null, randomAlphaOfLength(10)));
bulkRequestBuilder.add(new IndexRequestBuilder(null, randomAlphaOfLength(10)));
assertThat(bulkRequestBuilder.numberOfActions(), equalTo(3));
BulkRequest bulkRequest = bulkRequestBuilder.request();
assertNotNull(bulkRequest);
assertThat(bulkRequest.numberOfActions(), equalTo(3));
// Make sure that the bulk request builder is no longer holding onto the child request builders:
assertThat(bulkRequestBuilder.numberOfActions(), equalTo(0));
expectThrows(AssertionError.class, bulkRequestBuilder::request);
}
}