On Thu, 6 Mar 2025 02:18:35 GMT, Alex Menkov <amen...@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
