On Wed, 11 Oct 2023 10:57:24 GMT, Joachim Kern <jk...@openjdk.org> wrote:
>> We see rather often failures in java/lang/ProcessHandle/TreeTest.java on AIX
>> in TreeTest.test5.
>> The reason is: Previously the implementation based on the /proc file system
>> lead to double pids in the child list; at least intermittent. Using the API
>> getprocs64() instead I was able to eliminate those double pids (and increase
>> the performance by a factor of 4). Otherwise we would have to add an
>> algorithm to filter out the doubles after creating the raw list.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cosmetic changes 2
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 69:
> 67:
> 68: if (arraySize != parentArraySize) {
> 69: JNU_ThrowIllegalArgumentException(env, "array sizes not
> equal");
proposal: "Parent pid array must have the same length as result pid array"
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 89:
> 87:
> 88: do { // Block to break out of on Exception
> 89: pids = (*env)->GetLongArrayElements(env, jarray, NULL);
Nit, I'd move these invocations of GLAE into the initialization block and
remove the outer loop. I see what you do here, the outer block gives you a
reliable jumping point to get cleanup in case of an error. But I would just use
a goto cleanup label. That's clearer and allows you to handle initialization in
one place.
Proposal for initialization:
if (jparentArray != null) {
- check size is same as primary array, if wrong throw + goto cleanup
- ppids = GLAE
} else {
- if input pid is 0, throw an error too, since for "get all processes mode"
we need the parent array. Throw + goto cleanup.
}
// same for times array
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 112:
> 110: jlong startTime = 0L;
> 111:
> 112: /* skip files that aren't numbers */
As @MBaesken pointed out, this comment is obsolete.
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 114:
> 112: /* skip files that aren't numbers */
> 113: pid_t childpid = (pid_t) ProcessBuffer[i].pi_pid;
> 114: if ((int) childpid <= 0) {
Can this even happen? Negative pids are group pids, I don't think getprocs
returns group pids.
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 120:
> 118: // Get the parent pid, and start time
> 119: ppid = (pid_t) ProcessBuffer[i].pi_ppid;
> 120: startTime = ((jlong) ProcessBuffer[i].pi_start) *1000;
nit: space before 1000
src/java.base/aix/native/libjava/ProcessHandleImpl_aix.c line 121:
> 119: ppid = (pid_t) ProcessBuffer[i].pi_ppid;
> 120: startTime = ((jlong) ProcessBuffer[i].pi_start) *1000;
> 121: if (ppid >= 0 && (pid == 0 || ppid == pid)) {
I think the first condition is always true. You can remove it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356131767
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356134470
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137028
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137647
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356137949
PR Review Comment: https://git.openjdk.org/jdk/pull/16051#discussion_r1356141163
