Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Thu, 1 Feb 2024 05:42:07 GMT, David Holmes wrote: >> Hello David, I had actually first put it in that line you noted, but that >> then lead to a compilation error: >> >> >> In file included from >> /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33: >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error: >> unknown type name 'FILE' >> void print_message(FILE *fp, const char *prefix, const char *suffix, >>^ >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: >> a parameter list without types is only allowed in a function definition >> const char * jvmtiErrorText(jvmtiError); >> ^ >> /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: >> a parameter list without types is only allowed in a function definition >> const char * jdwpErrorText(jdwpError); >>^ >> 3 errors generated. >> >> >> >> I think the reason for those compilation errors is that `error_messages.h` >> and a few other header files are not explicitly listing their necessary >> includes and are relying on transitive includes. So I decided to take the >> easy route out, by adding the `error_messages.h` after the `util.h` to rely >> on the transitive includes to get past that compilation error. >> >> I think to properly fix that compile error, this following change (which I >> tested locally and works) would be needed: >> >> >> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> index 92a9f457e8a..2683c6791a1 100644 >> --- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h >> @@ -26,6 +26,8 @@ >> #ifndef JDWP_JDWP_H >> #define JDWP_JDWP_H >> >> +#include "jni_md.h" >> +#include "jvmti.h" >> #include "JDWPCommands.h" >> >> /* >> diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> index 4126b76f226..810ab384f1a 100644 >> --- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> +++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h >> @@ -26,6 +26,9 @@ >> #ifndef JDWP_ERROR_MESSAGES_H >> #define JDWP_ERROR_MESSAGES_H >> >> +#include >> +#include "JDWP.h" >> + >> /* It is assumed that ALL strings are UTF-8 safe on entry */ >> #define TTY_MESSAGE(args) ( tty_message args ) >> #define ERROR_MESSAGE(args) ( \ >> diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c >> b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c >> index b43d4de80... > > Please file a JBS issue, but no need for you to address it. Thanks. I've created https://bugs.openjdk.org/browse/JDK-8325094 to track this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1473868787
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 07:52:18 GMT, Jaikiran Pai wrote: >> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35: >> >>> 33: #include "sys.h" >>> 34: #include "util.h" >>> 35: #include "error_messages.h" >> >> Nit: to maintain include sort order this should have gone where >> `log_messages.h` was removed. > > Hello David, I had actually first put it in that line you noted, but that > then lead to a compilation error: > > > In file included from > /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33: > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error: > unknown type name 'FILE' > void print_message(FILE *fp, const char *prefix, const char *suffix, >^ > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: a > parameter list without types is only allowed in a function definition > const char * jvmtiErrorText(jvmtiError); > ^ > /jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: a > parameter list without types is only allowed in a function definition > const char * jdwpErrorText(jdwpError); >^ > 3 errors generated. > > > > I think the reason for those compilation errors is that `error_messages.h` > and a few other header files are not explicitly listing their necessary > includes and are relying on transitive includes. So I decided to take the > easy route out, by adding the `error_messages.h` after the `util.h` to rely > on the transitive includes to get past that compilation error. > > I think to properly fix that compile error, this following change (which I > tested locally and works) would be needed: > > > diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > index 92a9f457e8a..2683c6791a1 100644 > --- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > +++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h > @@ -26,6 +26,8 @@ > #ifndef JDWP_JDWP_H > #define JDWP_JDWP_H > > +#include "jni_md.h" > +#include "jvmti.h" > #include "JDWPCommands.h" > > /* > diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > index 4126b76f226..810ab384f1a 100644 > --- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > +++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h > @@ -26,6 +26,9 @@ > #ifndef JDWP_ERROR_MESSAGES_H > #define JDWP_ERROR_MESSAGES_H > > +#include > +#include "JDWP.h" > + > /* It is assumed that ALL strings are UTF-8 safe on entry */ > #define TTY_MESSAGE(args) ( tty_message args ) > #define ERROR_MESSAGE(args) ( \ > diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > index b43d4de80fc..cf0d00db192 100644 > --- a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c > +++ b/src/jdk.jdwp.agent... Please file a JBS issue, but no need for you to address it. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1473821677
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 06:28:05 GMT, David Holmes wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
>
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 100:
>
>> 98: if ((dp = opendir(FD_DIR)) == NULL) {
>> 99: ERROR_MESSAGE(("failed to open dir %s while determining"
>> 100: " file descriptors to close for process %d", FD_DIR,
>> getpid()));
>
> Nit: indent of second line is now off - please align "
Done. PR has been update to fix this.
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 139:
>
>> 137: ERROR_MESSAGE(("failed to close file descriptors of"
>> 138: " child process optimally, falling back to closing"
>> 139: " %d file descriptors sequentially", (max_fd - i +
>> 1)));
>
> Nit: please realign lines on "
Done
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472430580
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472430298
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 06:27:33 GMT, David Holmes wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
>
> src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35:
>
>> 33: #include "sys.h"
>> 34: #include "util.h"
>> 35: #include "error_messages.h"
>
> Nit: to maintain include sort order this should have gone where
> `log_messages.h` was removed.
Hello David, I had actually first put it in that line you noted, but that then
lead to a compilation error:
In file included from /jdk/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c:33:
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:35:20: error:
unknown type name 'FILE'
void print_message(FILE *fp, const char *prefix, const char *suffix,
^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:41:29: error: a
parameter list without types is only allowed in a function definition
const char * jvmtiErrorText(jvmtiError);
^
/jdk/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h:43:28: error: a
parameter list without types is only allowed in a function definition
const char * jdwpErrorText(jdwpError);
^
3 errors generated.
I think the reason for those compilation errors is that `error_messages.h` and
a few other header files are not explicitly listing their necessary includes
and are relying on transitive includes. So I decided to take the easy route
out, by adding the `error_messages.h` after the `util.h` to rely on the
transitive includes to get past that compilation error.
I think to properly fix that compile error, this following change (which I
tested locally and works) would be needed:
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
index 92a9f457e8a..04f10bcb019 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/JDWP.h
@@ -27,6 +27,7 @@
#define JDWP_JDWP_H
#include "JDWPCommands.h"
+#include "vm_interface.h"
/*
* JDWPCommands.h is the javah'ed version of all the constants defined
diff --git a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
index 4126b76f226..810ab384f1a 100644
--- a/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
+++ b/src/jdk.jdwp.agent/share/native/libjdwp/error_messages.h
@@ -26,6 +26,9 @@
#ifndef JDWP_ERROR_MESSAGES_H
#define JDWP_ERROR_MESSAGES_H
+#include
+#include "JDWP.h"
+
/* It is assumed that ALL strings are UTF-8 safe on entry */
#define TTY_MESSAGE(args) ( tty_message args )
#define ERROR_MESSAGE(args) ( \
diff --git a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
index 44f8a463522..0c6ee5b8b6a 100644
--- a/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
+++ b/src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
@@ -30,9 +30,9 @@
#include
#include
#include
+#include "error_messages.h"
#include "sys.h"
#include "util.h"
-#include "error_messages.h"
static char *skipWhitespace(char *p) {
while ((*p != '\0') && isspace(*p)) {
Do you think this should be fixed? I can file a JBS issue for it and address it
as a separate PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472425903
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 01:46:57 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address
>> https://bugs.openjdk.org/browse/JDK-8324668?
>>
>> This change proposes to fix the issue in jdwp where when launching a child
>> process (for the `launch=` option), it iterates over an extremely large
>> number of file descriptors to close each one of those. Details about the
>> issue and the proposed fixed are added in a subsequent comment in this PR
>> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1,
>> tier2 and tier3 testing is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE
A couple of minor nits to fix before integrating but otherwise looks good.
Thanks again for taking this on.
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 35:
> 33: #include "sys.h"
> 34: #include "util.h"
> 35: #include "error_messages.h"
Nit: to maintain include sort order this should have gone where
`log_messages.h` was removed.
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 100:
> 98: if ((dp = opendir(FD_DIR)) == NULL) {
> 99: ERROR_MESSAGE(("failed to open dir %s while determining"
> 100: " file descriptors to close for process %d", FD_DIR,
> getpid()));
Nit: indent of second line is now off - please align "
src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 139:
> 137: ERROR_MESSAGE(("failed to close file descriptors of"
> 138: " child process optimally, falling back to closing"
> 139: " %d file descriptors sequentially", (max_fd - i +
> 1)));
Nit: please realign lines on "
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1852976350
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472358835
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472359191
PR Review Comment: https://git.openjdk.org/jdk/pull/17588#discussion_r1472359972
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 01:46:57 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE Thank you Chris, David and Gerard for the inputs and reviews. I'll integrate this in a day or two once I'm able to have a full CI run done. - PR Comment: https://git.openjdk.org/jdk/pull/17588#issuecomment-1918248456
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
On Wed, 31 Jan 2024 01:46:57 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address >> https://bugs.openjdk.org/browse/JDK-8324668? >> >> This change proposes to fix the issue in jdwp where when launching a child >> process (for the `launch=` option), it iterates over an extremely large >> number of file descriptors to close each one of those. Details about the >> issue and the proposed fixed are added in a subsequent comment in this PR >> https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, >> tier2 and tier3 testing is currently in progress with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE Looks good. Thanks for taking this on. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17588#pullrequestreview-1852749213
Re: RFR: 8324668: JDWP process management needs more efficient file descriptor handling [v6]
> Can I please get a review of this change which proposes to address > https://bugs.openjdk.org/browse/JDK-8324668? > > This change proposes to fix the issue in jdwp where when launching a child > process (for the `launch=` option), it iterates over an extremely large > number of file descriptors to close each one of those. Details about the > issue and the proposed fixed are added in a subsequent comment in this PR > https://github.com/openjdk/jdk/pull/17588#issuecomment-1912256075. tier1, > tier2 and tier3 testing is currently in progress with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Chris' review suggestion - replace LOG_MISC with ERROR_MESSAGE - Changes: - all: https://git.openjdk.org/jdk/pull/17588/files - new: https://git.openjdk.org/jdk/pull/17588/files/c1decbff..3d323f7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17588&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17588&range=04-05 Stats: 4 lines in 1 file changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17588.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17588/head:pull/17588 PR: https://git.openjdk.org/jdk/pull/17588
