RFR: 8319055: JCMD should not buffer the whole output of commands

[Johan Sjölen]

On Fri, 31 Jan 2025 23:23:36 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> The fix implements streaming output support for attach protocol.
> See JBS issue for evaluation, summary of the changes in the 1st comment.
> Testing: tier1..4,hs-tier5-svc

Hi Alex,

Thank you for this. It's very good to have this feature. I'm still reading the code, but I'm submitting these comments as a first round.

I didn't really understand what makes this streaming. The old protocol first sent out the result, and then the data, has this changed?

Thanks,
Johan

src/hotspot/share/services/attachListener.cpp line 53:

> 51: 
> 52: // Stream for printing attach operation results.
> 53: // Supports buffered and streaming output for commands which can produce lenghtly reply.

"lengthy"

src/hotspot/share/services/attachListener.cpp line 56:

> 54: //
> 55: // To support streaming output platform implementation need to implement AttachOperation::get_reply_writer() method
> 56: // and ctor allow_streaming argument should be set to true.

Strange mix of "need" and "should".

Can we split this into multiple sentences? Here's a suggestion, though I'm not sure if it's 100% correct :-).


An output platform implementation supports streaming if it implements AttachOperation::get_reply_writer().
Streaming is enabled if the allow_streaming in the constructor is set to true.

src/hotspot/share/services/attachListener.cpp line 139:

> 137:       }
> 138:     } else {
> 139:       /* TODO: handle buffer overflow

Important `TODO` :-).

src/hotspot/share/services/diagnosticFramework.cpp line 389:

> 387:   int count = 0;
> 388:   while (iter.has_next()) {
> 389:     if(_source == DCmd_Source_MBean && count > 0) {

Pre-existing style: Space between `if` and condition missing.

-------------

PR Review: https://git.openjdk.org/jdk/pull/23405#pullrequestreview-2592338074
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940877174
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940882015
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940886089
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940875712