RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow

[David Holmes]

On Thu, 14 Dec 2023 10:13:51 GMT, Jaikiran Pai <jpai at openjdk.org> 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 `<pid>` 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.

Just some passing comments. I'm not familiar enough with the existing logic.

src/hotspot/os/posix/perfMemory_posix.cpp line 434:

> 432: // shared memory region for the given user name and vmid.
> 433: //
> 434: // the caller is expected to free the allocated memory.

Nit: comments with punctuation (e.g. terminating period) should be written as sentences and start with a capital letter e.g.

// either do this
// Or this.

Thanks

src/hotspot/os/posix/perfMemory_posix.cpp line 606:

> 604:     }
> 605:     // skip over files that are not regular files.
> 606:     if (!S_ISREG(statbuf.st_mode)) {

These appear to be able to be combined into a single if block.

src/hotspot/os/posix/perfMemory_posix.cpp line 612:

> 610:       continue;
> 611:     }
> 612:     FREE_C_HEAP_ARRAY(char, filename);

If you move this to immediately after lstat then you don't need it in the if-block

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

PR Review: https://git.openjdk.org/jdk/pull/17104#pullrequestreview-1783152470
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427568288
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427589543
PR Review Comment: https://git.openjdk.org/jdk/pull/17104#discussion_r1427590464