On Fri, 5 Dec 2025 09:42:51 GMT, Sergey Chernyshev <[email protected]> wrote:
>> We had a private discussion with Larry on this PR and undesired dependency >> on the `sun.jvmstat` module. >> We got the following concerns: >> - Do we really need 3x implementations of the MacOS `tempdir` finder >> function? >> - Do we really need a modular dependency from `jdk.attach` to >> `jdk.internal.jvmstat`? >> New method `getTemporaryDirectories(int pid)` is placed into the >> `jdk.internal.jvmstat/macosx/classes/sun/jvmstat/PlatformSupportImpl.java`. >> It seems it was needed to inherit the `PlatformSupport` class which has the >> method `PlatformSupport.getTemporaryDirectory()`. But it returns the same as >> the removed function >> `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`. So, this >> implementation can be moved to the `jdk.attach` module. >> My understanding is that class `PlatformSupportImpl` and its method >> `getTemporaryDirectories(int pid)` is not currently used for `jvmstat` >> implementation. Is it correct? Is it right that the `jvmstat` implementation >> still does not properly support root user on macOS? >> Anyway, some unification and refactoring is needed here. We could file a >> follow up enhancement to sort this out but also wanted to know your opinion >> on this. > > Hi @sspitsyn , > > Thank you for looking at this patch! > >> This is a nice fix in general. Thank you for this work! I hope to complete >> my review tomorrow with one more pass. Question: How was this update tested? >> Do you have a jtreg test or you've done it manually? Also, did you run mach5 >> tiers 1-6 to be safe and protected against regressions? > > The patch was tested against the current jtreg test groups (tier1-3) on macOS > both amd64 and arm64. I also run jcmd and jps tools manually under root, > because the change only affects the behavior under root. > >> * It seems it was needed to inherit the `PlatformSupport` class which has >> the method `PlatformSupport.getTemporaryDirectory()`. But it returns the >> same as the removed function >> `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`. So, this >> implementation can be moved to the `jdk.attach` module. > > The removed function `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()` > would return always the same string cached inside the > libsystem_coreservices.dylib, that would correspond the user specific > temporary path as of how the JVM process has started, that would have nothing > to do with the target process/pid temporary directory. > >> * My understanding is that class `PlatformSupportImpl` and its method >> `getTemporaryDirectories(int pid)` is not currently used for `jvmstat` >> implementation. Is it correct? > > I think this is not exactly so. `getTemporaryDirectories(int pid)` is used > currently in the LocalVmManager to get the list of active VMs, that is > consumed by `jcmd`, `jps` and `jconsole` utilities. When 0 is passed as a > parameter, it returns the list of temp directories for all processes, if they > were different (for example if there are containerized JVM processes under > Linux). > >> * Is it right that the `jvmstat` implementation still does not properly >> support root user on macOS? > > Yes and this is what this patch was intended for. > >> * Do we really need 3x implementations of the MacOS `tempdir` finder >> function? > > It was already 2x implementation of HotSpot perfdata files lookup, HotSpot > itself uses the `os::get_temp_directory()`, on the java side the > LocalVmManager in `sun.jvmstat.perfdata.monitor` relies on > `getTemporaryDirectories(int pid)` in the PlatformSupport. The 3rd was the > macOS native `Java_sun_tools_attach_VirtualMachineImpl_getTempDir()`, now > it's proposed for removal as it returns only the current user temp path, no > matter what process is being attached to. > >> * Do we really need a modular dependency f... @sercher Thank you for the readability updates and answering questions! ------------- PR Comment: https://git.openjdk.org/jdk/pull/25824#issuecomment-3631118258
