Stop retaining transport responses for test-only listener #125163

original-brownbear · 2025-03-18T20:48:06Z

We only retained the response to response to a test-only call back in this lambda. Also in OutboundMessage we only retain the message for its class name until the very end on the off chance we need it to print a slowness warning. This should do away with all retention in the transport layer code, probably a lot of spots to fix for this upstream from there, but IMO it's a good start and saves heap for all the spots that don't need fixing right away.

PS: I'm aware that the OutBoundMessage is a weird class now, even weirder than before. My suggestion would be to go for this change as a "shortest possible fix" kinda thing and refactor away OutBoundMessage in the next step (and quickly do so).

We only retained the response to response to a test-only call back in this lambda. With this fixed, all that's seemingly left is removing the response reference from `OutboundMessage` and we should be (unless there's more specifc to the callsite issues upstream) able to GC these potentially huge instances a lot quicker under network pressure.

elasticsearchmachine · 2025-03-18T20:48:39Z

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

…port-response-past-sending

DaveCTurner

Makes sense to me. Could you open a PR with the more finished state too (with OutboundMessage gone) so we can see more clearly where this is heading? It might be that the next step is also not so large and we can do it in one.

DaveCTurner · 2025-03-19T08:57:57Z

server/src/main/java/org/elasticsearch/transport/OutboundMessage.java

-                + "}{"
-                + isHandshake()
-                + "}{"
-                + message.getClass()


I'd rather we kept some way to identify the response type, although I must say I never much liked sharing its class here. Could we plumb in the action from the TcpTransportChannel down to here first?

…port-response-past-sending

We only retained the response to response to a test-only call back in this lambda. With this fixed, all that's seemingly left is removing the response reference from `OutboundMessage` and we should be (unless there's more specifc to the callsite issues upstream) able to GC these potentially huge instances a lot quicker under network pressure.

original-brownbear · 2025-03-20T15:38:10Z

@DaveCTurner thanks for taking a look. So roughly https://github.com/elastic/elasticsearch/compare/main...original-brownbear:drop-outbound-msg?expand=1 is where I'd take this. It's essentially just a bunch of test plumbing and maybe some other simplifications to serialization but nothing complicated either.

Note that I made that version in a way that preserves the slowness warning serialization exactly as it is today ... not necessarily the approach I'd recommend long term but also not a big deal memory wise for now.

Nobody uses this parameter (except some tests that simplify verify the otherwise-unused plumbing is connected). This commit removes it. Relates elastic#125163

Nobody uses this parameter (except some tests that simply verify the otherwise-unused plumbing is connected). This commit removes it. Relates elastic#125163

DaveCTurner · 2025-03-20T16:10:00Z

Ok yeah that looks nicer indeed, I think I'd rather do the whole thing in one step tbh rather than having this intermediate state. Or rather I'd like to do #125326 separately first because it's kinda unrelated to the rest of it, then just https://github.com/elastic/elasticsearch/compare/main...original-brownbear:drop-outbound-msg?expand=1 straight away.

original-brownbear · 2025-03-20T16:13:26Z

Sounds good, I'll wait for that PR to go on, then I'll update this one to cover all the changes when resolving the conflicts anyway :)

DaveCTurner · 2025-03-20T16:20:52Z

👍 I think my only comment so far is I'd rather not use requestAction == null to say whether we're sending a request or a response, let's have a boolean parameter instead (and validate it against requestAction == null)

Nobody uses this parameter (except some tests that simply verify the otherwise-unused plumbing is connected). This commit removes it. Relates #125163

…port-response-past-sending

…se-past-sending

…e-past-sending' into stop-retaining-transport-response-past-sending

original-brownbear · 2025-03-20T23:49:04Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java

            channel,
            message,
-            networkMessage,
+            requestAction == null


As I said above, not 100% sure about this one, but then again, either we want this information or not though and this still seems cheaper than redundantly building the string.

Yep I think we want this info. Looks easy enough to plumb in the action on the response path too these days, but we can do that in a followup.

original-brownbear · 2025-03-20T23:49:56Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java


+    // public for tests
+    public static BytesReference serialize(
+        @Nullable String requestAction,


I get your point @DaveCTurner, setting request or response based on a null is not great, but then again, this method already has an absurd number of parameters? :)

Maybe it needs a parameters object? I think I'd call it... OutboundMessage 🤣

I'll follow up with something like this:

original-brownbear/elasticsearch@stop-retaining-transport-response-past-sending...DaveCTurner:elasticsearch:2025/03/21/OutboundHandler-MessageDirection

drops the isError boolean that only makes sense on responses, replacing it with a three-state enum, and makes the action available on all paths.

original-brownbear · 2025-03-20T23:51:30Z

server/src/test/java/org/elasticsearch/transport/InboundDecoderTests.java

-            final BytesReference totalBytes = message.serialize(body, os);
+            final BytesReference totalBytes;
+            if (isRequest) {
+                totalBytes = OutboundHandler.serialize(


I guess we could add some overloads to shorten this thing, but not sure it's worth it, it's not that many spots and it's test only code.

Yeah no big deal IMO

original-brownbear · 2025-03-20T23:54:17Z

Alright thanks @DaveCTurner I merge my other branch into this one. Fine by me adding another flag here to mark a request but other than that wdyt? :) Certainly could be done nicer here and there, but IMO it's an improvement over the status quo beyond the GC improvements because it also makes it a little easier to add additional zero-copy serialization paths I think.

DaveCTurner

LGTM (few nits but nothing blocking)

DaveCTurner · 2025-03-21T03:17:59Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java


+    // public for tests
+    public static BytesReference serialize(
+        @Nullable String requestAction,


Maybe it needs a parameters object? I think I'd call it... OutboundMessage 🤣

DaveCTurner · 2025-03-21T03:18:54Z

server/src/test/java/org/elasticsearch/transport/InboundDecoderTests.java

-            final BytesReference totalBytes = message.serialize(body, os);
+            final BytesReference totalBytes;
+            if (isRequest) {
+                totalBytes = OutboundHandler.serialize(


Yeah no big deal IMO

DaveCTurner · 2025-03-21T03:20:12Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java

            channel,
            message,
-            networkMessage,
+            requestAction == null


Yep I think we want this info. Looks easy enough to plumb in the action on the response path too these days, but we can do that in a followup.

DaveCTurner · 2025-03-21T03:21:17Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java

     */
    void sendBytes(TcpChannel channel, BytesReference bytes, ActionListener<Void> listener) {
-        internalSend(channel, bytes, null, listener);
+        internalSend(channel, bytes, () -> "", listener);


Suggested change

internalSend(channel, bytes, () -> "", listener);

internalSend(channel, bytes, () -> "raw bytes", listener);

DaveCTurner · 2025-03-21T03:26:36Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java

+        if (compressionScheme != null) {
+            status = TransportStatus.setCompress(status);
+        }
+        byteStreamOutput.seek(0);


We should at least assert that byteStreamOutput.position() == 0 at the top of the method (the existing code also makes this assumption)

DaveCTurner · 2025-03-21T03:29:15Z

server/src/main/java/org/elasticsearch/transport/OutboundMessage.java

-
-import java.io.IOException;
-
-abstract class OutboundMessage extends NetworkMessage {


There's no usages of NetworkMessage left now either?

Right deleted it :)

DaveCTurner · 2025-03-21T03:31:37Z

server/src/main/java/org/elasticsearch/transport/OutboundHandler.java

+
+        final int variableHeaderLength = Math.toIntExact(byteStreamOutput.position() - TcpHeader.HEADER_SIZE);
+        BytesReference message = serializeMessageBody(writeable, compressionScheme, version, byteStreamOutput);
+        byte status = requestAction != null ? 0 : TransportStatus.setResponse((byte) 0);


nit: would prefer to start with 0 and use if (...) over ... ? ... : ... for harmony with the following lines

original-brownbear · 2025-03-21T12:07:29Z

Thanks David!

elasticsearchmachine · 2025-03-21T12:10:20Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 125163

…125326) Nobody uses this parameter (except some tests that simply verify the otherwise-unused plumbing is connected). This commit removes it. Relates elastic#125163

Remove the `OutboundMessage` class that needlessly holds on to the the response instances after they are not needed any longer. Inlining the logic should save considerably heap under pressure and enabled further optimisations.

…125326) Nobody uses this parameter (except some tests that simply verify the otherwise-unused plumbing is connected). This commit removes it. Relates elastic#125163

Remove the `OutboundMessage` class that needlessly holds on to the the response instances after they are not needed any longer. Inlining the logic should save considerably heap under pressure and enabled further optimisations.

) Remove the `OutboundMessage` class that needlessly holds on to the the response instances after they are not needed any longer. Inlining the logic should save considerably heap under pressure and enabled further optimisations. backport of #125163

original-brownbear · 2025-04-02T13:20:53Z

back ported in #126078 now, sorry for the delay @DaveCTurner !

DaveCTurner · 2025-04-02T14:00:46Z

Thanks Armin

original-brownbear added 2 commits March 18, 2025 10:05

bckl

fd49527

original-brownbear added >non-issue :Distributed Coordination/Network Http and internode communication implementations v8.19.0 v9.1.0 labels Mar 18, 2025

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Mar 18, 2025

Merge remote-tracking branch 'elastic/main' into stop-retaining-trans…

9897475

…port-response-past-sending

DaveCTurner reviewed Mar 19, 2025

View reviewed changes

original-brownbear added 3 commits March 19, 2025 13:24

Merge remote-tracking branch 'elastic/main' into stop-retaining-trans…

b034274

…port-response-past-sending

all the way

681783e

original-brownbear requested a review from DaveCTurner March 20, 2025 15:38

DaveCTurner mentioned this pull request Mar 20, 2025

Remove redundant response parameter to onResponseSent() #125326

Merged

Merge branch 'main' into stop-retaining-transport-response-past-sending

79cb99e

original-brownbear added 4 commits March 20, 2025 20:50

Merge remote-tracking branch 'elastic/main' into stop-retaining-trans…

04cb7d6

…port-response-past-sending

Merge remote-tracking branch 'elastic/main' into stop-retaining-trans…

17c451e

…port-response-past-sending

Merge branch 'drop-outbound-msg' into stop-retaining-transport-respon…

4bee100

…se-past-sending

Merge remote-tracking branch 'origin/stop-retaining-transport-respons…

7a34d93

…e-past-sending' into stop-retaining-transport-response-past-sending

original-brownbear commented Mar 20, 2025

View reviewed changes

DaveCTurner approved these changes Mar 21, 2025

View reviewed changes

original-brownbear added 3 commits March 21, 2025 08:04

CR: comments

835639a

CR: comments

af89489

Merge branch 'main' into stop-retaining-transport-response-past-sending

410040b

original-brownbear added the auto-backport Automatically create backport pull requests when merged label Mar 21, 2025

original-brownbear merged commit 9c8750b into elastic:main Mar 21, 2025
17 checks passed

original-brownbear deleted the stop-retaining-transport-response-past-sending branch March 21, 2025 12:08

elasticsearchmachine added the backport pending label Mar 21, 2025

DaveCTurner mentioned this pull request Mar 21, 2025

Record action name for all outbound messages #125399

Merged

original-brownbear mentioned this pull request Mar 26, 2025

Introduce batched query execution and data-node side reduce #121885

Merged

original-brownbear mentioned this pull request Apr 1, 2025

Stop retaining transport responses past serialization (#125163) #126078

Merged

original-brownbear removed the backport pending label Apr 2, 2025

original-brownbear mentioned this pull request Apr 21, 2025

Introduce BytesTransportResponse to enable lower heap overhead network responses #127112

Merged

original-brownbear mentioned this pull request May 8, 2025

Fix SearchErrorTraceIT and friends to work with batched query execution #127150

Closed

	internalSend(channel, bytes, () -> "", listener);
	internalSend(channel, bytes, () -> "raw bytes", listener);


		import java.io.IOException;

		abstract class OutboundMessage extends NetworkMessage {

Stop retaining transport responses for test-only listener #125163

Stop retaining transport responses for test-only listener #125163

Uh oh!

Conversation

original-brownbear commented Mar 18, 2025

Uh oh!

elasticsearchmachine commented Mar 18, 2025

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Mar 20, 2025

Uh oh!

DaveCTurner commented Mar 20, 2025

Uh oh!

original-brownbear commented Mar 20, 2025

Uh oh!

DaveCTurner commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Mar 20, 2025

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Mar 21, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Mar 21, 2025

💔 Backport failed

Uh oh!

original-brownbear commented Apr 2, 2025

Uh oh!

DaveCTurner commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants