Releasing child request builder memory from BulkRequestBuilder

[masseyke]

One disadvantage of the builder changes made at #104927 is that the peak memory usage is higher since when BulkRequestBuilder's request() method is called, we have to have the memory to hold onto all of the request builders for the child requests as well as all child requests themselves. This PR changes that peak memory usage by releasing each child request builder as soon as the child request has been created, so we're never in a situation where we have to hold all child request builders and all child requests in memory at once. It has the side effect that the request() method can only be called once. That does not seem inconsistent with the idea of a builder or our usage of it though. It's a pattern we've discussed enforcing on all of our builders.

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[joegallo]

The code looks fine to me, but I'd like the formal 👍/👎 review to come from somebody with more knowledge of the memory profile of a running Elasticsearch process and thus the consequences of this PR (i.e. somebody from @elastic/es-distributed).

[DaveCTurner]

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]

Or maybe use a queue instead?

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

[arteam]

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]

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]

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]

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]

Oops, right. :)

[DaveCTurner]

LGTM

[DaveCTurner]

nit: may as well not waste half a line spelling out the type signature :)

Suggested change

	for (ActionRequestLazyBuilder<? extends DocWriteRequest<?>, ? extends DocWriteResponse> builder = requestBuilders
	for (final var builder = requestBuilders