Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote: > After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Class loading order is different to class initialization order. There are a lot more tests than just tier1. :) I don't expect many, if any, tests to be looking for a specific IOOBE message, and I can't see an easy way to find such tests without running them. If core-libs folk are okay with this update then I guess we can just handle any test failures if they arise. But I'll run this through our tier 1-3 testing. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268857: Merge three vm operations in thread_dump [v2]
> Hi, > > Could I have a review of this change that merges three vm > operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump and > signal_thread_entry. > > `jstack` is a very common command, even in the production environment. > > In addition to reduce the cost of entering safepoint, I think this patch also > could ensure the consistency of the results of VM_PrintThreads and > VM_FindDeadlocks. Denghui Dong has updated the pull request incrementally with three additional commits since the last revision: - update - remove useless comment - remove VM_PrintJNI - Changes: - all: https://git.openjdk.java.net/jdk/pull/4504/files - new: https://git.openjdk.java.net/jdk/pull/4504/files/82a35b85..56cd2a8b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4504=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4504=00-01 Stats: 38 lines in 5 files changed: 4 ins; 28 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4504.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4504/head:pull/4504 PR: https://git.openjdk.java.net/jdk/pull/4504
Re: Monitoring Java Safepoint Time in JDK16+
On 17/06/2021 02:39, David Holmes wrote: In what way does this no longer work? We have a test in jdk/sun/management/HotspotRuntimeMBean/GetTotalSafepointTime.java that uses this API and doesn't seem to need to do anything to allow access. ?? That test is compiled/run with `--add-exports java.management/sun.management=ALL-UNNAMED`, just not immediately obvious because the TEST.properties with the config for jtreg is in the parent directory (it applies to tests in that directory and all sub-directories). Carter would have need the equivalent to compile with JDK 9+ and to run with JDK 16+. -Alan.
Re: RFR: 8268857: Merge three vm operations in thread_dump
On Wed, 16 Jun 2021 03:33:14 GMT, Denghui Dong wrote: > Hi, > > Could I have a review of this change that merges three vm > operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump and > signal_thread_entry. > > `jstack` is a very common command, even in the production environment. > > In addition to reduce the cost of entering safepoint, I think this patch also > could ensure the consistency of the results of VM_PrintThreads and > VM_FindDeadlocks. It would be nice to have this fixed though. Maybe adding a guaranteed timeout to these operations would help. - PR: https://git.openjdk.java.net/jdk/pull/4504
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 01:51:41 GMT, David Holmes wrote: > I skimmed through all these and the changes seem fine in principal. > I have two mild concerns: > > 1. How does this change the class initialization order on VM startup? > 2. Do any tests need adjusting due to potential changes in the exact message > used by the IndexOutOfBoundsException? > > Thanks, > David Hi David, your concerns are reasonable. I think this change would not affect the class initialization order, because regardless of whether the patch is applied or not, `java -Xlog:class+load -version` prints identical class initialization order(for j.l.Objects and jdk.internal.util.Preconditions) as far as I can see. [class_load.log](https://github.com/openjdk/jdk/files/6667168/class_load.log). And tier1 tests are all passed w/o any modifications. - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268857: Merge three vm operations in thread_dump
On Wed, 16 Jun 2021 04:08:10 GMT, David Holmes wrote: > Sorry but I do not agree with this change. I understand your rationale but > you have overlooked that by combining these three safepoint operations you > have created a much longer safepoint pause which may lock out other more > important safepoint operations, and you may also be holding the HeapLock > across this extended safepoint. > > The operations that you have coalesced are not considered time critical but > can themselves be very time consuming e.g deadlock detection. > > David Hi David, Thanks for the comment. I still think it's redundant to enter three safepoint, which may lead to inconsistent results between 'PrintThreads' and 'FindDeadlocks'. But this patch does cause the problem you said I will reopen it once I have a better solution. Thanks, Denghui - PR: https://git.openjdk.java.net/jdk/pull/4504
Withdrawn: 8268857: Merge three vm operations in thread_dump
On Wed, 16 Jun 2021 03:33:14 GMT, Denghui Dong wrote: > Hi, > > Could I have a review of this change that merges three vm > operations(VM_PrintThreads, VM_PrintJNI, VM_FindDeadlocks) in thread_dump and > signal_thread_entry. > > `jstack` is a very common command, even in the production environment. > > In addition to reduce the cost of entering safepoint, I think this patch also > could ensure the consistency of the results of VM_PrintThreads and > VM_FindDeadlocks. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/4504
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote: > After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. I skimmed through all these and the changes seem fine in principal. I have two mild concerns: 1. How does this change the class initialization order on VM startup? 2. Do any tests need adjusting due to potential changes in the exact message used by the IndexOutOfBoundsException? Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: Monitoring Java Safepoint Time in JDK16+
Hi Carter, On 17/06/2021 5:12 am, Carter Kozak wrote: As java 16 and beyond lock down access to internal components by default, it can be difficult to produce Prometheus-style metrics describing application safepoints. I’ve been monitoring these metrics so that I can be alerted when an application spends more than ~10% of time in safepoints for some duration, because it means that something has gone wrong: Most often GC spirals, however excessive thread dumps, deadlock checks, etc can contribute. This has been one of the most meaningful tools in my arsenal to detect general JVM badness, however there doesn’t seem to be a way to access the data in newer JREs without allowing access to internal components. Previously I was able to use something along these lines, which required legacy sun.management component access. sun.management.HotspotRuntimeMBean hotspotRuntimeManagementBean = sun.management.ManagementFactoryHelper.getHotspotRuntimeMBean(); long totalSafepointTimeMillis = hotspotRuntimeManagementBean.getTotalSafepointTime(); In what way does this no longer work? We have a test in jdk/sun/management/HotspotRuntimeMBean/GetTotalSafepointTime.java that uses this API and doesn't seem to need to do anything to allow access. ?? Cheers, David - Before I get ahead of myself, I’d like to confirm that I haven’t missed a supported pathway to access safepoint time. If my read is correct and there’s no way to access this information from inside the running JVM, would it be a a reasonable addition to the public API? Thanks, Carter Kozak
[jdk17] Integrated: 8268863: ProblemList serviceability/sa/TestJmapCoreMetaspace.java on linux-x64 with ZGC
On Wed, 16 Jun 2021 05:32:51 GMT, Yasumasa Suenaga wrote: > In order to reduce the noise in the JDK17 CI, I'm ProblemListing > serviceability/sa/TestJmapCoreMetaspace.java on linux-x64 with ZGC. This pull request has now been integrated. Changeset: ee03bc6d Author:Yasumasa Suenaga URL: https://git.openjdk.java.net/jdk17/commit/ee03bc6d0aad4b0d07bbe792c8cc77f986c617e1 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8268863: ProblemList serviceability/sa/TestJmapCoreMetaspace.java on linux-x64 with ZGC Reviewed-by: dcubed - PR: https://git.openjdk.java.net/jdk17/pull/72
Re: Monitoring Java Safepoint Time in JDK16+
Thanks, Erik.
Is there any chance you could point me toward the documentation for the
jdk.Safepoint* events? It's difficult to tell at a glance if the sum of the
differences between the safepoint begin and end times is equivalent to
hotspotRuntimeManagementBean.getTotalSafepointTime(). Does that include the
sync time (which appears to have its own SafepointStateSynchronization event)?
I don't understand what beginSafepointEvent.getDuration() means in this
context, is sync time included only in the safepoint sync event, duration of
the SafepointBegin event, or time between begin and end event end times?
In logging framework code we've found that converting between time objects and
numeric values doesn't always (on some jres ever?) optimize away Instant
allocations. The default JFR configuration appears to set time thresholds for
safepoint events which I'd expect reduces overhead, but would prevent us from
accumulating data when there are frequent, quick safepoints. Otherwise the cost
is likely to be much greater than the old approach.
In a similar vein, is there any way for us to accumulate this data without
dedicating an entire OS thread to the RecordingStream?
Sorry for the barrage of questions, I appreciate your help!
Carter Kozak
On Wed, Jun 16, 2021, at 15:44, Erik Gahlin wrote:
> It's possible to access safepoint information using JFR, for example:
>
> try (RecordingStream r = new RecordingStream()) {
> r.enable("jdk.SafepointBegin");
> r.enable("jdk.SafepointEnd");
> r.onEvent("jdk.SafepointBegin", e -> System.out.println("begin: " +
> e.getEndTime()));
> r.onEvent("jdk.SafepointEnd", e -> System.out.println("end: " +
> e.getEndTime()));
> r.start();
> }
>
> Erik
>
> On 2021-06-16, 21:12, "serviceability-dev on behalf of Carter Kozak"
> on behalf of
> cko...@ckozak.net > wrote:
>
> As java 16 and beyond lock down access to internal components by default,
> it can be difficult to produce Prometheus-style metrics describing
> application safepoints. I’ve been monitoring these metrics so that I can be
> alerted when an application spends more than ~10% of time in safepoints for
> some duration, because it means that something has gone wrong: Most often GC
> spirals, however excessive thread dumps, deadlock checks, etc can contribute.
> This has been one of the most meaningful tools in my arsenal to detect
> general JVM badness, however there doesn’t seem to be a way to access the
> data in newer JREs without allowing access to internal components.
>
> Previously I was able to use something along these lines, which required
> legacy sun.management component access.
>
> sun.management.HotspotRuntimeMBean hotspotRuntimeManagementBean =
> sun.management.ManagementFactoryHelper.getHotspotRuntimeMBean();
> long totalSafepointTimeMillis =
> hotspotRuntimeManagementBean.getTotalSafepointTime();
>
> Before I get ahead of myself, I’d like to confirm that I haven’t missed a
> supported pathway to access safepoint time. If my read is correct and there’s
> no way to access this information from inside the running JVM, would it be a
> a reasonable addition to the public API?
>
> Thanks,
> Carter Kozak
>
>
>
[jdk17] Withdrawn: 8268433: serviceability/dcmd/framework/VMVersionTest.java fails with Unable to send object throw not established PipeIO Listener Thread connection
On Fri, 11 Jun 2021 19:40:09 GMT, Alex Menkov wrote: > The fix adds check that IOPipe is connected before sending "quit" command This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk17/pull/32
Re: [jdk17] RFR: 8268433: serviceability/dcmd/framework/VMVersionTest.java fails with Unable to send object throw not established PipeIO Listener Thread connection
On Fri, 11 Jun 2021 19:40:09 GMT, Alex Menkov wrote: > The fix adds check that IOPipe is connected before sending "quit" command The tests are problem-listed in jdk17 and I have problems with testing windows+zgc builds Closing this PR and going to continue in jdk18 - PR: https://git.openjdk.java.net/jdk17/pull/32
Re: Monitoring Java Safepoint Time in JDK16+
It's possible to access safepoint information using JFR, for example:
try (RecordingStream r = new RecordingStream()) {
r.enable("jdk.SafepointBegin");
r.enable("jdk.SafepointEnd");
r.onEvent("jdk.SafepointBegin", e -> System.out.println("begin: " +
e.getEndTime()));
r.onEvent("jdk.SafepointEnd", e -> System.out.println("end: " +
e.getEndTime()));
r.start();
}
Erik
On 2021-06-16, 21:12, "serviceability-dev on behalf of Carter Kozak"
wrote:
As java 16 and beyond lock down access to internal components by default,
it can be difficult to produce Prometheus-style metrics describing application
safepoints. I’ve been monitoring these metrics so that I can be alerted when an
application spends more than ~10% of time in safepoints for some duration,
because it means that something has gone wrong: Most often GC spirals, however
excessive thread dumps, deadlock checks, etc can contribute. This has been one
of the most meaningful tools in my arsenal to detect general JVM badness,
however there doesn’t seem to be a way to access the data in newer JREs without
allowing access to internal components.
Previously I was able to use something along these lines, which required
legacy sun.management component access.
sun.management.HotspotRuntimeMBean hotspotRuntimeManagementBean =
sun.management.ManagementFactoryHelper.getHotspotRuntimeMBean();
long totalSafepointTimeMillis =
hotspotRuntimeManagementBean.getTotalSafepointTime();
Before I get ahead of myself, I’d like to confirm that I haven’t missed a
supported pathway to access safepoint time. If my read is correct and there’s
no way to access this information from inside the running JVM, would it be a a
reasonable addition to the public API?
Thanks,
Carter Kozak
Monitoring Java Safepoint Time in JDK16+
As java 16 and beyond lock down access to internal components by default, it can be difficult to produce Prometheus-style metrics describing application safepoints. I’ve been monitoring these metrics so that I can be alerted when an application spends more than ~10% of time in safepoints for some duration, because it means that something has gone wrong: Most often GC spirals, however excessive thread dumps, deadlock checks, etc can contribute. This has been one of the most meaningful tools in my arsenal to detect general JVM badness, however there doesn’t seem to be a way to access the data in newer JREs without allowing access to internal components. Previously I was able to use something along these lines, which required legacy sun.management component access. sun.management.HotspotRuntimeMBean hotspotRuntimeManagementBean = sun.management.ManagementFactoryHelper.getHotspotRuntimeMBean(); long totalSafepointTimeMillis = hotspotRuntimeManagementBean.getTotalSafepointTime(); Before I get ahead of myself, I’d like to confirm that I haven’t missed a supported pathway to access safepoint time. If my read is correct and there’s no way to access this information from inside the running JVM, would it be a a reasonable addition to the public API? Thanks, Carter Kozak
Re: [jdk17] RFR: 8268863: ProblemList serviceability/sa/TestJmapCoreMetaspace.java on linux-x64 with ZGC
On Wed, 16 Jun 2021 05:32:51 GMT, Yasumasa Suenaga wrote: > In order to reduce the noise in the JDK17 CI, I'm ProblemListing > serviceability/sa/TestJmapCoreMetaspace.java on linux-x64 with ZGC. Thumbs up. This is a trivial fix. - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/72
Re: RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 13:10:55 GMT, Evgeny Astigeevich wrote: >> This change removes the mark_for_evol_deoptimization method and removes the >> flag that all dependencies are recorded. Before the change to walk the >> entire nmethod looking for "old" (redefined) methods with metadata_do(), we >> used to find methods in the code cache to deoptimize based on evol_method >> dependencies. If the dependencies weren't yet recorded, we had to >> deoptimize all of the methods. A long time ago, we had a customer who was >> unhappy with the pause for this when they had late attach. Now we don't >> have this problem. >> The evol_method dependencies are still used by the compiler to check for old >> methods during compilation. I didn't change this but it might be something >> someone who knows the compiler better can do differently and remove these >> dependencies too. >> Tested with tier1-6. > > LGTM Thanks @eastig . - PR: https://git.openjdk.java.net/jdk/pull/4509
Re: RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 12:52:46 GMT, Coleen Phillimore wrote: > This change removes the mark_for_evol_deoptimization method and removes the > flag that all dependencies are recorded. Before the change to walk the > entire nmethod looking for "old" (redefined) methods with metadata_do(), we > used to find methods in the code cache to deoptimize based on evol_method > dependencies. If the dependencies weren't yet recorded, we had to deoptimize > all of the methods. A long time ago, we had a customer who was unhappy with > the pause for this when they had late attach. Now we don't have this problem. > The evol_method dependencies are still used by the compiler to check for old > methods during compilation. I didn't change this but it might be something > someone who knows the compiler better can do differently and remove these > dependencies too. > Tested with tier1-6. LGTM - PR: https://git.openjdk.java.net/jdk/pull/4509
Re: RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
On Wed, 16 Jun 2021 12:52:46 GMT, Coleen Phillimore wrote: > This change removes the mark_for_evol_deoptimization method and removes the > flag that all dependencies are recorded. Before the change to walk the > entire nmethod looking for "old" (redefined) methods with metadata_do(), we > used to find methods in the code cache to deoptimize based on evol_method > dependencies. If the dependencies weren't yet recorded, we had to deoptimize > all of the methods. A long time ago, we had a customer who was unhappy with > the pause for this when they had late attach. Now we don't have this problem. > The evol_method dependencies are still used by the compiler to check for old > methods during compilation. I didn't change this but it might be something > someone who knows the compiler better can do differently and remove these > dependencies too. > Tested with tier1-6. Hi @iwanowww can you have a look? - PR: https://git.openjdk.java.net/jdk/pull/4509
RFR: 8264941: Remove CodeCache::mark_for_evol_deoptimization() method
This change removes the mark_for_evol_deoptimization method and removes the flag that all dependencies are recorded. Before the change to walk the entire nmethod looking for "old" (redefined) methods with metadata_do(), we used to find methods in the code cache to deoptimize based on evol_method dependencies. If the dependencies weren't yet recorded, we had to deoptimize all of the methods. A long time ago, we had a customer who was unhappy with the pause for this when they had late attach. Now we don't have this problem. The evol_method dependencies are still used by the compiler to check for old methods during compilation. I didn't change this but it might be something someone who knows the compiler better can do differently and remove these dependencies too. Tested with tier1-6. - Commit messages: - 8264941: Remove CodeCache::mark_for_evol_deoptimization() method Changes: https://git.openjdk.java.net/jdk/pull/4509/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4509=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264941 Stats: 78 lines in 7 files changed: 0 ins; 73 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/4509.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4509/head:pull/4509 PR: https://git.openjdk.java.net/jdk/pull/4509
RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
After JDK-8265518, it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase. - Commit messages: - use checkIndex globally Changes: https://git.openjdk.java.net/jdk/pull/4507/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4507=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268698 Stats: 206 lines in 34 files changed: 31 ins; 111 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
