Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Thu, 4 Nov 2021 02:15:45 GMT, Ioi Lam wrote: >> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs >> owned by a specific user. No one uses these APIs, and they don't work anyway. >> >> The current code is very confusing to look at. Since we're likely to change >> code in this area for further container support, it's better to clean up the >> code now. >> >> - Remove all APIs that take a user name >> - Also removed PerfDataFile.getFile() methods that are unused >> - Cleaned up the code that looks up the hsperfdata_xxx files >> - Fix comments to explain what's happening >> - Avoid using Matcher.reset which is not thread-safe >> - Renamed confusing variables such as `userFilter` to make the code more >> readable >> - LocalVmManager.activeVms() probably doesn't need to be synchronized, but >> I kept it anyway to avoid unnecessary risks. >> >> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers >> and have extensive use of the management tools). > > Ioi Lam 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 three additional commits since > the last revision: > > - Merge branch 'master' into > 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code > - @kevinjwalls and @plummercj review - (1) restore > PerfDataFile.userDirNamePattern, etc. (2) Fixed comments > - 8275185: Remove dead code and clean up jvmstat LocalVmManager Supporting 1.4.1 hsperfdata seems pointless at this point, so I support a removal of that code. It also seems reasonable to do as a follow-up, as @iklam suggests, rather than shifting the scope this PR. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5923
RFR: 8276628: Use blessed modifier order in serviceability code
I ran bin/blessed-modifier-order.sh on source owned by serviceability. This scripts verifies that modifiers are in the "blessed" order, and fixes it otherwise. I have manually checked the changes made by the script to make sure they are sound. - Commit messages: - 8276628: Use blessed modifier order in serviceability code Changes: https://git.openjdk.java.net/jdk/pull/6249/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6249&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276628 Stats: 235 lines in 89 files changed: 0 ins; 0 del; 235 mod Patch: https://git.openjdk.java.net/jdk/pull/6249.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6249/head:pull/6249 PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Thu, 4 Nov 2021 02:15:45 GMT, Ioi Lam wrote: >> LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs >> owned by a specific user. No one uses these APIs, and they don't work anyway. >> >> The current code is very confusing to look at. Since we're likely to change >> code in this area for further container support, it's better to clean up the >> code now. >> >> - Remove all APIs that take a user name >> - Also removed PerfDataFile.getFile() methods that are unused >> - Cleaned up the code that looks up the hsperfdata_xxx files >> - Fix comments to explain what's happening >> - Avoid using Matcher.reset which is not thread-safe >> - Renamed confusing variables such as `userFilter` to make the code more >> readable >> - LocalVmManager.activeVms() probably doesn't need to be synchronized, but >> I kept it anyway to avoid unnecessary risks. >> >> Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers >> and have extensive use of the management tools). > > Ioi Lam 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 three additional commits since > the last revision: > > - Merge branch 'master' into > 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code > - @kevinjwalls and @plummercj review - (1) restore > PerfDataFile.userDirNamePattern, etc. (2) Fixed comments > - 8275185: Remove dead code and clean up jvmstat LocalVmManager Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Thu, 4 Nov 2021 02:07:42 GMT, Ioi Lam wrote: > ...moved the filename patterns back to PerfDataFile.java... Thanks, looks good! - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: 8276628: Use blessed modifier order in serviceability code
On Thu, 4 Nov 2021 10:44:41 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source owned by serviceability. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. Looks fine to me. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6249
RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
`VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out reflection invocation targets for delegating reflection class loaders. Post JEP 416 we don't use DelegatingClassLoaders anymore. This patch removes the display of reflection targets from these commands as well as associated helper code and tests. I don't have enough time atm to reimplement this feature using method handles. But at least we can remove the old code, and prepare the way for more code removal. The patch does not touch vmClasses, `reflect_ConstructorAccessor` and `reflect_MethodAccessor` are both still there. Tests: GHAs, manually testing the commands. - Commit messages: - Remove reflection invocation target printing from VM.metaspace, VM.classloaders, VM.class_hierarchy Changes: https://git.openjdk.java.net/jdk/pull/6257/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6257&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272065 Stats: 368 lines in 8 files changed: 0 ins; 367 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6257.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6257/head:pull/6257 PR: https://git.openjdk.java.net/jdk/pull/6257
Re: RFR: 8276628: Use blessed modifier order in serviceability code
On Thu, 4 Nov 2021 10:44:41 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source owned by serviceability. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. Marked as reviewed by lmesnik (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8276628: Use blessed modifier order in serviceability code
On Thu, 4 Nov 2021 10:44:41 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source owned by serviceability. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. Copyright updates? - PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8276173: Clean up and remove unneeded casts in HeapDumper [v2]
On Tue, 2 Nov 2021 19:25:40 GMT, Leo Korinth wrote: >> HeapDumper does a lot of unneeded casts. Some arguments should be const. >> Headers are not correctly sorted. Comment about identifier size on Windows >> and Solaris is not true. >> >> First I cleaned up casting in the "union casting", but then I decided it was >> better to create a temporary bit_cast that we can use until we get the >> proper one in c++20. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > restart failed github tests This looks good. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6211
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v11]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr3.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/3e1d1b06..86fcdfbe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=09-10 Stats: 10 lines in 3 files changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @robehn - Thanks for your reviews on the previous version (8249004.cr2.patch). Mach5 Tier[1-5] testing has finished and looks good; Mach5 Tier[678] are still running. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 13:25:14 GMT, Thomas Stuefe wrote: > `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out > reflection invocation targets for delegating reflection class loaders. Post > JEP 416 we don't use DelegatingClassLoaders anymore. > > This patch removes the display of reflection targets from these commands as > well as associated helper code and tests. > > I don't have enough time atm to reimplement this feature using method > handles. But at least we can remove the old code, and prepare the way for > more code removal. > > The patch does not touch vmClasses, `reflect_ConstructorAccessor` and > `reflect_MethodAccessor` are both still there. > > Tests: GHAs, manually testing the commands. Looks good to me. Thanks for following this up.The new implementation does not spin any new class loader and so I don't think jcmd needs to extend its support for the new implementation using method handles. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6257
Re: RFR: 8276628: Use blessed modifier order in serviceability code
On Thu, 4 Nov 2021 15:59:48 GMT, Chris Plummer wrote: >> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This >> scripts verifies that modifiers are in the "blessed" order, and fixes it >> otherwise. I have manually checked the changes made by the script to make >> sure they are sound. > > Copyright updates? @plummercj That's really kind of an edge case, considering the triviality of these changes. But yes, the norm in the JDK is to update the copyright year for all changes, so you're right, I should do that. (And we *really* ought to wrangle free some resources to write tooling for us to do all the copyright dances...) - PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:16:03 GMT, David Holmes wrote: >> I suspect that the way that git is displaying the diffs is confusing you. >> >> We need `current_thread` set if we get to line 474 so we have to init >> `current_thread` on line 446 for the `checkTLHOnly == true` case and >> on line 463 for the `checkTLHOnly == false` case. >> >> I could simplify the logic by always setting current thread when it is >> declared on 444, but I was trying to avoid the call to `Thread::current()` >> until I actually needed it. I thought `Thread::current()` can be expensive. >> Is this no longer the case? > > Sorry I missed that line 463 is still within the else from line 447. > > Thread::current() is a compiler-defined thread-local access so should be > relatively cheap these days, but I have no numbers. I'm really tempted to go ahead and change it to always set current thread when it is declared and then clean things up a bit. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:18:23 GMT, David Holmes wrote:
>> The rationale for removing the is_exiting() check from `java_suspend()` was
>> that it
>> was redundant because the handshake code detected and handled the
>> `is_exiting()`
>> case so we didn't need to do that work twice.
>>
>> If we look at `HandshakeState::resume()` there is no logic for detecting or
>> handling
>> the possibility of an exiting thread. That being said, we have to look
>> closer at what
>> `HandshakeState::resume()` does and whether that logic can be harmful if
>> executed
>> on an exiting thread.
>>
>> Here's the code:
>>
>> bool HandshakeState::resume() {
>> if (!is_suspended()) {
>> return false;
>> }
>> MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
>> if (!is_suspended()) {
>> assert(!_handshakee->is_suspended(), "cannot be suspended without a
>> suspend request");
>> return false;
>> }
>> // Resume the thread.
>> set_suspended(false);
>> _lock.notify();
>> return true;
>> }
>>
>>
>> I'm not seeing anything in `HandshakeState::resume()` that
>> worries me with respect to an exiting thread. Of course, the
>> proof is in the testing so I'll rerun the usual testing after
>> deleting that code.
>
> A suspended thread cannot be exiting - else the suspend logic is broken. So,
> given you can call `resume()` on a not-suspended thread, as long as the
> handshake code checks for `is_supended()` (which it does) then no explicit
> `is_exiting` check is needed.
Agreed! I have to keep reminding myself that with handshake based suspend
and resume, we just don't have the same races with exiting threads that we
used to have.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8276173: Clean up and remove unneeded casts in HeapDumper [v2]
On Tue, 2 Nov 2021 19:25:40 GMT, Leo Korinth wrote: >> HeapDumper does a lot of unneeded casts. Some arguments should be const. >> Headers are not correctly sorted. Comment about identifier size on Windows >> and Solaris is not true. >> >> First I cleaned up casting in the "union casting", but then I decided it was >> better to create a temporary bit_cast that we can use until we get the >> proper one in c++20. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > restart failed github tests Looks good! Thanks, Harold - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6211
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Thu, 4 Nov 2021 05:12:32 GMT, Thomas Stuefe wrote: >> It's actually fairly common to have Mac-specific stuff in the BSD files. The >> macOS >> port was built on top of the BSD port and the BSD port was built by copying >> a LOT >> of code from Linux into BSD specific files with modifications as needed. >> >> If I pushed this change down into MachDecoder, then I would have to lose the >> `ShouldNotReachHere()` call in order to not assert in non-release bits. I >> don't >> think I want to do that since this may not be the only place that calls the >> 6-arg version of decode(). > >> It's actually fairly common to have Mac-specific stuff in the BSD files. The >> macOS port was built on top of the BSD port and the BSD port was built by >> copying a LOT of code from Linux into BSD specific files with modifications >> as needed. > > I always wondered whether anyone actually builds the BSDs in head. I assume > Oracle does not, right? I know there are downstream porters somewhere but > only for old releases, or? > >> >> If I pushed this change down into MachDecoder, then I would have to lose the >> `ShouldNotReachHere()` call in order to not assert in non-release bits. I >> don't think I want to do that since this may not be the only place that >> calls the 6-arg version of decode(). > > Fair enough, thanks for the clarification. Oracle does not build BSD in head. At one point, Dmitry Samersoff used to build BSD in his lab, but I don't know if he still does that. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. @tstuefe - Thanks for closing the loop on my previous replies. @gerard-ziemski - Thanks for the review! I'm going to make more tweaks to this fix and will update the PR after my test cycle is complete. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Thu, 4 Nov 2021 05:12:48 GMT, Thomas Stuefe wrote: >> I took a quick look at the other calls to `dladdr()` in >> src/hotspot/os/bsd/os_bsd.cpp >> and I'm not comfortable with changing those uses without having a specific >> test >> case that I can use to investigate those code paths. >> >> We are fairly early in our testing on macOS12 so I may run into a reason to >> revisit >> this choice down the road. > >> I took a quick look at the other calls to `dladdr()` in >> src/hotspot/os/bsd/os_bsd.cpp and I'm not comfortable with changing those >> uses without having a specific test case that I can use to investigate those >> code paths. >> >> We are fairly early in our testing on macOS12 so I may run into a reason to >> revisit this choice down the road. > > Okay! I've had a chance to think about this overnight and I'm not liking my duplication of code so I'm going to look at adding a wrapper that is called by the two calls sites where know I need the special handling. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Wed, 3 Nov 2021 21:14:17 GMT, Gerard Ziemski wrote: >> macOS12 has changed the dladdr() function to accept "-1" as a valid address >> and >> we have functions that use dladdr() to convert DLL addresses into function or >> library names. We also have a gtest that verifies that "-1" is not a valid >> value to use >> as a symbol address. >> >> As you might imagine, existing code that uses >> `os::dll_address_to_function_name()` >> or `os::dll_address_to_library_name()` can get quite confused (and sometimes >> crash) >> if an `addr` parameter of `-1` was allowed to be used. >> >> I've also made two cleanup changes as part of this fix: >> >> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that >> should >>be properly `#ifdef`'ed. There is also some code that makes sense for ELF >> format >>files, but not for Mach-O format files so that code needs to be excluded >> on macOS. >> >> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a >> comment on an >> `#endif` that I fixed. That typo does not appear anywhere else in the >> HotSpot code >> base so I'd like to fix it with this bug ID since I'm in related areas. >> >> This fix has been tested with Mach5 Tier[1-6]. > > src/hotspot/os/bsd/os_bsd.cpp line 929: > >> 927: #endif >> 928: >> 929: #if defined(__APPLE__) > > Why not just do: > > `#else` > > here instead and collapse these 3 lines into 1? Hmmm... I'll take a look at doing that. > src/hotspot/os/bsd/os_bsd.cpp line 930: > >> 928: >> 929: #if defined(__APPLE__) >> 930: char localbuf[MACH_MAXSYMLEN]; > > This `__APPLE__` section is the only one, that I can see, using > `MACH_MAXSYMLEN`, why not move: > > > #if defined(__APPLE__) > #define MACH_MAXSYMLEN 256 > #endif > > > here (i.e. just the `#define MACH_MAXSYMLEN 256` and minimize the need for` > __APPLE__` sections? Hmmm I'll take a look at cleaning this up a bit. > src/hotspot/os/bsd/os_bsd.cpp line 964: > >> 962: if (offset) *offset = -1; >> 963: return false; >> 964: } > > Do we need this here? Wouldn't the earlier call to `Decoder::decode(addr, > localbuf, MACH_MAXSYMLEN, offset, dlinfo.dli_fbase))` catch this with > `ShouldNotReachHere`? That's the 5-parameter version of decode() and it doesn't have `ShouldNotReachHere`. So if that code site is called and returns `false`, then we get into `dll_address_to_library_name()` and reach this `dladdr()` call which will accept the "-1"... - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8276628: Use blessed modifier order in serviceability code [v2]
> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This > scripts verifies that modifiers are in the "blessed" order, and fixes it > otherwise. I have manually checked the changes made by the script to make > sure they are sound. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Also update copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/6249/files - new: https://git.openjdk.java.net/jdk/pull/6249/files/cf5db105..8a5ff8a5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6249&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6249&range=00-01 Stats: 49 lines in 49 files changed: 0 ins; 0 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/6249.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6249/head:pull/6249 PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8276628: Use blessed modifier order in serviceability code [v2]
On Thu, 4 Nov 2021 17:36:52 GMT, Magnus Ihse Bursie wrote: >> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This >> scripts verifies that modifiers are in the "blessed" order, and fixes it >> otherwise. I have manually checked the changes made by the script to make >> sure they are sound. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update copyright year Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 17:02:05 GMT, Mandy Chung wrote: > Looks good to me. Thanks for following this up. The new implementation does > not spin any new class loader and so I don't think jcmd needs to extend its > support for the new implementation using method handles. Thank you Mandy! - PR: https://git.openjdk.java.net/jdk/pull/6257
Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 13:25:14 GMT, Thomas Stuefe wrote: > `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out > reflection invocation targets for delegating reflection class loaders. Post > JEP 416 we don't use DelegatingClassLoaders anymore. > > This patch removes the display of reflection targets from these commands as > well as associated helper code and tests. > > I don't have enough time atm to reimplement this feature using method > handles. But at least we can remove the old code, and prepare the way for > more code removal. > > The patch does not touch vmClasses, `reflect_ConstructorAccessor` and > `reflect_MethodAccessor` are both still there. > > Tests: GHAs, manually testing the commands. Yes, looks good to me also. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6257
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr2.patch Sorry for such a long delay looking at this. I had a couple questions and a suggestion. - Changes requested by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 21:34:50 GMT, Daniel D. Daugherty wrote: >> src/hotspot/share/prims/jvmtiEventController.cpp line 623: >> >>> 621: // If we have a JvmtiThreadState, then we've reached the point >>> where >>> 622: // threads can exist so create a ThreadsListHandle to protect them. >>> 623: ThreadsListHandle tlh; >> >> Good catch on the missing TLH for this code. > > It wasn't quite missing from the baseline code. This version of execute(): > > `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` > > used to always create a ThreadsListHandle. I added a `ThreadsListHandle*` > parameter to that version and created a wrapper with the existing signature > to pass `nullptr` to the execute() version with the `ThreadsListHandle*` > parameter. What that means is that all existing callers of: > > `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` > > no longer had a ThreadsListHandle created for them. With the new sanity > check in place, I shook the trees to make sure that we had explicit > ThreadsListHandles in place for the locations that needed them. > > `JvmtiEventControllerPrivate::recompute_enabled()` happened to be > one of the places where the ThreadsListHandle created by execute() > was hiding the fact that `recompute_enabled()` needed one. Should the ThreadsListHandle protect JvmtiThreadState::first also? If state->next() needs it why doesn't the first entry need this? There's no atomic load on the _head field. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 17:02:59 GMT, Daniel D. Daugherty wrote: >> Sorry I missed that line 463 is still within the else from line 447. >> >> Thread::current() is a compiler-defined thread-local access so should be >> relatively cheap these days, but I have no numbers. > > I'm really tempted to go ahead and change it to always set > current thread when it is declared and then clean things up a bit. Yes, ^ that might make the logic easier to follow. I can't figure out what checkTLSOnly means. Could it be refactored into a different function like check_TLS() and then call it in the place where you pass true instead of is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Thu, 4 Nov 2021 05:13:29 GMT, Thomas Stuefe wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8273967.cr1.patch > > Marked as reviewed by stuefe (Reviewer). @tstuefe and @gerard-ziemski - please re-review when you get the chance. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12
On Mon, 1 Nov 2021 15:32:54 GMT, Daniel D. Daugherty wrote: > macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. This version has been tested with Mach5 Tier1 and with runs of the specific tests using release and debug bits on macosx-aarch64 and macosx-x64 test machines running macOS12. - PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
> macOS12 has changed the dladdr() function to accept "-1" as a valid address > and > we have functions that use dladdr() to convert DLL addresses into function or > library names. We also have a gtest that verifies that "-1" is not a valid > value to use > as a symbol address. > > As you might imagine, existing code that uses > `os::dll_address_to_function_name()` > or `os::dll_address_to_library_name()` can get quite confused (and sometimes > crash) > if an `addr` parameter of `-1` was allowed to be used. > > I've also made two cleanup changes as part of this fix: > > 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that > should >be properly `#ifdef`'ed. There is also some code that makes sense for ELF > format >files, but not for Mach-O format files so that code needs to be excluded > on macOS. > > 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a comment > on an > `#endif` that I fixed. That typo does not appear anywhere else in the > HotSpot code > base so I'd like to fix it with this bug ID since I'm in related areas. > > This fix has been tested with Mach5 Tier[1-6]. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8273967.cr1.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/6193/files - new: https://git.openjdk.java.net/jdk/pull/6193/files/f57d7f0d..ecb230d6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6193&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6193&range=00-01 Stats: 48 lines in 1 file changed: 16 ins; 28 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6193/head:pull/6193 PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: 8276628: Use blessed modifier order in serviceability code [v2]
On Thu, 4 Nov 2021 17:36:52 GMT, Magnus Ihse Bursie wrote: >> I ran bin/blessed-modifier-order.sh on source owned by serviceability. This >> scripts verifies that modifiers are in the "blessed" order, and fixes it >> otherwise. I have manually checked the changes made by the script to make >> sure they are sound. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update copyright year Good catch on copyright updates @plummercj ! We always have to update when we modify a file - the one exception is changing the wording of the copyright notice itself. Looks good. David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6249
Re: RFR: 8275185: Remove dead code and clean up jvmstat LocalVmManager [v2]
On Thu, 4 Nov 2021 03:44:43 GMT, Chris Plummer wrote: >> Ioi Lam 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 three additional commits >> since the last revision: >> >> - Merge branch 'master' into >> 8275185-jvmstat-LocalVmManager-cleanup-and-remove-dead-code >> - @kevinjwalls and @plummercj review - (1) restore >> PerfDataFile.userDirNamePattern, etc. (2) Fixed comments >> - 8275185: Remove dead code and clean up jvmstat LocalVmManager > > Marked as reviewed by cjplummer (Reviewer). Thanks @plummercj @kevinjwalls @cl4es for the review. I filed https://bugs.openjdk.java.net/browse/JDK-8276687 to remove the JDK 1.4.1 support. Passed Oracle CI tiers1-4 - PR: https://git.openjdk.java.net/jdk/pull/5923
Integrated: 8275185: Remove dead code and clean up jvmstat LocalVmManager
On Wed, 13 Oct 2021 04:17:25 GMT, Ioi Lam wrote: > LocalVmManager and PerfDataFile have APIs that are supposed to look for VMs > owned by a specific user. No one uses these APIs, and they don't work anyway. > > The current code is very confusing to look at. Since we're likely to change > code in this area for further container support, it's better to clean up the > code now. > > - Remove all APIs that take a user name > - Also removed PerfDataFile.getFile() methods that are unused > - Cleaned up the code that looks up the hsperfdata_xxx files > - Fix comments to explain what's happening > - Avoid using Matcher.reset which is not thread-safe > - Renamed confusing variables such as `userFilter` to make the code more > readable > - LocalVmManager.activeVms() probably doesn't need to be synchronized, but > I kept it anyway to avoid unnecessary risks. > > Testing with Oracle CI: tiers1-4, plus hs-tier5-rt (which tests containers > and have extensive use of the management tools). This pull request has now been integrated. Changeset: 8e17ce00 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/8e17ce00316a765bbedefc34dc5898ba4f3f2144 Stats: 296 lines in 2 files changed: 9 ins; 255 del; 32 mod 8275185: Remove dead code and clean up jvmstat LocalVmManager Reviewed-by: cjplummer, redestad, kevinw - PR: https://git.openjdk.java.net/jdk/pull/5923
Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 13:25:14 GMT, Thomas Stuefe wrote: > `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out > reflection invocation targets for delegating reflection class loaders. Post > JEP 416 we don't use DelegatingClassLoaders anymore. > > This patch removes the display of reflection targets from these commands as > well as associated helper code and tests. > > I don't have enough time atm to reimplement this feature using method > handles. But at least we can remove the old code, and prepare the way for > more code removal. > > The patch does not touch vmClasses, `reflect_ConstructorAccessor` and > `reflect_MethodAccessor` are both still there. > > Tests: GHAs, manually testing the commands. I never realized we needed special handling for these classloaders so I'm glad to see this gone too. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6257
Re: RFR: 8273967: gtest os.dll_address_to_function_and_library_name_vm fails on macOS12 [v2]
On Thu, 4 Nov 2021 22:59:39 GMT, Daniel D. Daugherty wrote: >> macOS12 has changed the dladdr() function to accept "-1" as a valid address >> and >> we have functions that use dladdr() to convert DLL addresses into function or >> library names. We also have a gtest that verifies that "-1" is not a valid >> value to use >> as a symbol address. >> >> As you might imagine, existing code that uses >> `os::dll_address_to_function_name()` >> or `os::dll_address_to_library_name()` can get quite confused (and sometimes >> crash) >> if an `addr` parameter of `-1` was allowed to be used. >> >> I've also made two cleanup changes as part of this fix: >> >> 1) In `src/hotspot/os/bsd/os_bsd.cpp` there is some macOS specific code that >> should >>be properly `#ifdef`'ed. There is also some code that makes sense for ELF >> format >>files, but not for Mach-O format files so that code needs to be excluded >> on macOS. >> >> 2) In `src/hotspot/share/runtime/os.cpp` I noticed a simple typo in a >> comment on an >> `#endif` that I fixed. That typo does not appear anywhere else in the >> HotSpot code >> base so I'd like to fix it with this bug ID since I'm in related areas. >> >> This fix has been tested with Mach5 Tier[1-6]. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8273967.cr1.patch Looks good! - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6193
Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 17:02:05 GMT, Mandy Chung wrote: >> `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out >> reflection invocation targets for delegating reflection class loaders. Post >> JEP 416 we don't use DelegatingClassLoaders anymore. >> >> This patch removes the display of reflection targets from these commands as >> well as associated helper code and tests. >> >> I don't have enough time atm to reimplement this feature using method >> handles. But at least we can remove the old code, and prepare the way for >> more code removal. >> >> The patch does not touch vmClasses, `reflect_ConstructorAccessor` and >> `reflect_MethodAccessor` are both still there. >> >> Tests: GHAs, manually testing the commands. > > Looks good to me. Thanks for following this up.The new implementation > does not spin any new class loader and so I don't think jcmd needs to extend > its support for the new implementation using method handles. Thanks @mlchung, @coleenp and @dholmes-ora. - PR: https://git.openjdk.java.net/jdk/pull/6257
Integrated: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416
On Thu, 4 Nov 2021 13:25:14 GMT, Thomas Stuefe wrote: > `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out > reflection invocation targets for delegating reflection class loaders. Post > JEP 416 we don't use DelegatingClassLoaders anymore. > > This patch removes the display of reflection targets from these commands as > well as associated helper code and tests. > > I don't have enough time atm to reimplement this feature using method > handles. But at least we can remove the old code, and prepare the way for > more code removal. > > The patch does not touch vmClasses, `reflect_ConstructorAccessor` and > `reflect_MethodAccessor` are both still there. > > Tests: GHAs, manually testing the commands. This pull request has now been integrated. Changeset: 7281861e Author:Thomas Stuefe URL: https://git.openjdk.java.net/jdk/commit/7281861e0662e6c51507066a1f12673a236c7491 Stats: 368 lines in 8 files changed: 0 ins; 367 del; 1 mod 8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416 Reviewed-by: mchung, coleenp, dholmes - PR: https://git.openjdk.java.net/jdk/pull/6257
