Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 07:29:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to improve the code >> in `get_user_name_slow` function, which is used to identify the target JVM >> owner's user name? This addresses >> https://bugs.openjdk.org/browse/JDK-8321971. >> >> As noted in that JBS issue, in its current form, the nested loop ends up >> iterating over the directory contents of `hsperfdata_xxx` directory and then >> for each iteration it checks if the name of the entry matches the pid. This >> iteration shouldn't be needed and instead one could look for a file named >> `` within that directory. >> >> No new test has been added, given the nature of this change. Existing tier1, >> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and >> macosx. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - remove redundant if block > - merge latest from master branch > - David's review comments - reduce if blocks and release the array outside > if block > - David's review comment - punctuation > - 8321971: Improve the user name detection logic in perfMemory > get_user_name_slow Hello Johan, thank you for that input and sorry about the delay in responding. I got side tracked with a few other things. I plan to refresh this PR and address the comments, this week. - PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1964357753
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 12:18:36 GMT, Jaikiran Pai wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - remove redundant if block
>> - merge latest from master branch
>> - David's review comments - reduce if blocks and release the array outside
>> if block
>> - David's review comment - punctuation
>> - 8321971: Improve the user name detection logic in perfMemory
>> get_user_name_slow
>
> Just a note - I have incorporated the review comments, except from Johan
> which I'm still investigating and will update this PR soon.
Hi @jaikiran,
I looked at the `snprintf` issue, the easiest way of fixing this is this:
```c++
{
char* name = NEW_C_HEAP_ARRAY(char, nbytes, mtInternal);
int bytes_required = snprintf(name, nbytes, "%s/%d", dirname, pid);
if (bytes_required >= nbytes) {
// Our output was truncated
FREE_C_HEAP_ARRAY(name);
nbytes = bytes_required;
name = NEW_C_HEAP_ARRAY(char, nbytes, mtInternal);
bytes_required = snprintf(name, nbytes, "%s/%d", dirname, pid);
assert(bytes_required < nbytes, "must be");
}
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1953095597
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 07:29:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to improve the code >> in `get_user_name_slow` function, which is used to identify the target JVM >> owner's user name? This addresses >> https://bugs.openjdk.org/browse/JDK-8321971. >> >> As noted in that JBS issue, in its current form, the nested loop ends up >> iterating over the directory contents of `hsperfdata_xxx` directory and then >> for each iteration it checks if the name of the entry matches the pid. This >> iteration shouldn't be needed and instead one could look for a file named >> `` within that directory. >> >> No new test has been added, given the nature of this change. Existing tier1, >> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and >> macosx. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - remove redundant if block > - merge latest from master branch > - David's review comments - reduce if blocks and release the array outside > if block > - David's review comment - punctuation > - 8321971: Improve the user name detection logic in perfMemory > get_user_name_slow Just a note - I have incorporated the review comments, except from Johan which I'm still investigating and will update this PR soon. - PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1864379594
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
> Can I please get a review of this change which proposes to improve the code > in `get_user_name_slow` function, which is used to identify the target JVM > owner's user name? This addresses https://bugs.openjdk.org/browse/JDK-8321971. > > As noted in that JBS issue, in its current form, the nested loop ends up > iterating over the directory contents of `hsperfdata_xxx` directory and then > for each iteration it checks if the name of the entry matches the pid. This > iteration shouldn't be needed and instead one could look for a file named > `` within that directory. > > No new test has been added, given the nature of this change. Existing tier1, > tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and > macosx. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - remove redundant if block - merge latest from master branch - David's review comments - reduce if blocks and release the array outside if block - David's review comment - punctuation - 8321971: Improve the user name detection logic in perfMemory get_user_name_slow - Changes: - all: https://git.openjdk.org/jdk/pull/17104/files - new: https://git.openjdk.org/jdk/pull/17104/files/cfd50d79..243d9817 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17104&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17104&range=01-02 Stats: 3660 lines in 265 files changed: 2177 ins; 586 del; 897 mod Patch: https://git.openjdk.org/jdk/pull/17104.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17104/head:pull/17104 PR: https://git.openjdk.org/jdk/pull/17104
