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

[Johan Sjölen]

On Thu, 6 Mar 2025 02:18:35 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
>
> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   feedback

Hi Alex,

I'm generally happy with this code. I've got a few more comments, but nothing major.

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

> 314: 
> 315: static jint data_dump(AttachOperation* op, attachStream* out) {
> 316:   out->set_result(JNI_OK); // allow streaming output

I'm wondering, do you think it's useful to add a helper `start_streaming_if_available` (or whatever) that just calls `set_result(JNI_OK)`? That helper could have a small snippet of documentation to explain what's going on. That would probably be neater than a comment.

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

> 371:   // The commands report error if the agent failed to load, so we need to disable streaming output.
> 372:   const char jmx_prefix[] = "ManagementAgent.";
> 373:   if (strncmp(op->arg(0), jmx_prefix, strlen(jmx_prefix)) == 0) {

`const char* jmx_prefix = "ManagementAgent.", not array.

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

> 394:   if (HAS_PENDING_EXCEPTION) {
> 395:     // We can get an exception during command execution.
> 396:     // In the case _attach_stream->set_result() is already called and operation result code is send

"is senT to the client"

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line 379:

> 377: 
> 378:             // Reply is "<ver> option1,option2...".
> 379:             int delimPos = message.indexOf(' ');

We can do this in an early-break sort of way. That'd reduce the indentation and let's us not to have to check `delimPos` again.


int delimPos = message.indexOf(' ');
int supportedVersion = Integer.parseUnsignedInt(delimPos < 0 ? message : message.substring(0, delimPos));
if (delimPos < 0 || supportedVersion != VERSION_2) {
    return; // No options to parse, or doesn't support options anyway
}
/* Insert the parsing stuff here */

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

PR Review: https://git.openjdk.org/jdk/pull/23405#pullrequestreview-2665162130
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1983778660
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1983782768
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1983783438
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1983812437