Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
On Mon, 11 Apr 2022 23:41:40 GMT, David Holmes wrote: > This is wrong - it should be BITSET not BITMAP Oh, good find! I filed a follow-up issue: https://bugs.openjdk.java.net/browse/JDK-8284725 - PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Mon, 11 Apr 2022 07:52:34 GMT, Alan Bateman wrote: > > @AlanBateman adding the explicit compile commands to add `--enable-preview` > > is exactly what causes the problem. By compiling that individual file first > > it also compiled some testlib dependencies but puts the classes in a > > different place. When another class is later compiled, the compilation path > > includes that destination and so compilation succeeds, but at runtime the > > path is different and we get the NCDFE. This is the analysis that Alex did > > in a similar issue - see his comment in JBS. > > Adding `--enable-preview` did not intentionally mean to introduce implicit > compilation but it looks like we'll have to re-visit the ordering (as > @alexmenkov suggests) in the loom repo. I believe the underlying cause of this type of failure is https://bugs.openjdk.java.net/browse/CODETOOLS-7902847 "Class directory of a test case should not be used to compile a library" You can easily see the bug by adding this to test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.java in the mainline JDK repo: + * @compile RedefineRunningMethods.java * @run main RedefineClassHelper Clean up your jtreg "work" directory and run this test by itself. Afterwards, look for .class files in the jtreg work dir: ./work/classes/test/lib/RedefineClassHelper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$2.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$FileManagerWrapper$1.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$FileManagerWrapper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler$MemoryJavaFileObject.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/compiler/InMemoryJavaCompiler.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/helpers/ClassFileInstaller$Manifest.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/jdk/test/lib/helpers/ClassFileInstaller.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$1.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods$3.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineClassHelper.class ./work/classes/serviceability/jvmti/RedefineClasses/RedefineRunningMethods.d/RedefineRunningMethods_B.class You can see that the `@library /test/lib` is split into two parts: /work/classes/test/lib/ - Most of it is in the directory that's used only by the RedefineRunningMethods.java test case - Some of it is in the `work/classes/test/lib/` directory, which is shared by all test cases. Now, create a test case RedefineRunningMethods2.java in the same directory that looks like this: /* * @test * @requires vm.jvmti * @library /test/lib * @modules java.base/jdk.internal.misc * @modules java.compiler * java.instrument * jdk.jartool/sun.tools.jar * @run main RedefineClassHelper */ If you run these two test together, RedefineRunningMethods.java is executed first, leaving the work directory in the same condition as shown above. Then, jtreg runs RedefineRunningMethods2.java, and sees `work/classes/test/lib/RedefineClassHelper.class` is already there. So it executes RedefineClassHelper without rebuilding it, leading to a failure because classes that RedefineClassHelper depends on are not available to this second test. I have a very simple reproducer in the bug above with a detailed log that shows what is happening. While this problem could be worked around, I believe the right fix should be done inside jtreg. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284673: Collapse identical catch branches in java.management
Hello,
> new RuntimeException(exc1.getMessage())
I typically try to avoid swallowing the cause and especially the exception type
of a inner exception, especially in this case where two different exceptions
are caught. Better do tostring() and/or specify the exception as a cause to
have chaining?
--
https://bernd.eckenfels.net
From: serviceability-dev on behalf
of David Holmes
Sent: Tuesday, April 12, 2022 3:52:38 AM
To: serviceability-dev@openjdk.java.net
Subject: Re: RFR: 8284673: Collapse identical catch branches in java.management
On Fri, 8 Apr 2022 12:19:03 GMT, Andrey Turbanov wrote:
> Let's take advantage of Java 7 language feature - "Catching Multiple
> Exception Types".
> It simplifies code. Reduces duplication.
> Found by IntelliJ IDEA inspection Identical 'catch' branches in 'try'
> statement
Nice cleanup! Great to see the code duplication gone.
One minor additional edit requested below.
Thanks,
David
src/java.management/share/classes/javax/management/relation/RelationService.java
line 1920:
> 1918: } catch (InstanceNotFoundException | ReflectionException
> exc1) {
> 1919: throw new RuntimeException(exc1.getMessage());
> 1920: } catch (MBeanException exc3) {
Please change exc3 to exc2
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8161
Re: RFR: 8284673: Collapse identical catch branches in java.management
On Tue, 12 Apr 2022 00:12:42 GMT, Chris Plummer wrote: >> Let's take advantage of Java 7 language feature - "Catching Multiple >> Exception Types". >> It simplifies code. Reduces duplication. >> Found by IntelliJ IDEA inspection Identical 'catch' branches in 'try' >> statement > > src/java.management/share/classes/javax/management/modelmbean/RequiredModelMBean.java > line 1198: > >> 1196: throw new RuntimeOperationsException(ree, >> 1197: "RuntimeException occurred in RequiredModelMBean >> "+ >> 1198: "while trying to invoke operation " + opName); > > This one is a bit different. You've collapsed RuntimeOperationsException into > the RuntimeException handler, which seems fine since it is a subclass of > RuntimeException and both handlers do the same thing. However, I wonder if > the original intent of the RuntimeOperationsException handler was for it to > have a different message, and perhaps reference RuntimeOperationsException > instead of RuntimeException. The proposed change is behaviour preserving so I have no issue with it. If something else was intended then that would be a new RFE IMO. - PR: https://git.openjdk.java.net/jdk/pull/8161
Re: RFR: 8284673: Collapse identical catch branches in java.management
On Fri, 8 Apr 2022 12:19:03 GMT, Andrey Turbanov wrote:
> Let's take advantage of Java 7 language feature - "Catching Multiple
> Exception Types".
> It simplifies code. Reduces duplication.
> Found by IntelliJ IDEA inspection Identical 'catch' branches in 'try'
> statement
Nice cleanup! Great to see the code duplication gone.
One minor additional edit requested below.
Thanks,
David
src/java.management/share/classes/javax/management/relation/RelationService.java
line 1920:
> 1918: } catch (InstanceNotFoundException | ReflectionException
> exc1) {
> 1919: throw new RuntimeException(exc1.getMessage());
> 1920: } catch (MBeanException exc3) {
Please change exc3 to exc2
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8161
Re: RFR: 8284673: Collapse identical catch branches in java.management
On Fri, 8 Apr 2022 12:19:03 GMT, Andrey Turbanov wrote: > Let's take advantage of Java 7 language feature - "Catching Multiple > Exception Types". > It simplifies code. Reduces duplication. > Found by IntelliJ IDEA inspection Identical 'catch' branches in 'try' > statement Marked as reviewed by cjplummer (Reviewer). src/java.management/share/classes/javax/management/modelmbean/RequiredModelMBean.java line 1198: > 1196: throw new RuntimeOperationsException(ree, > 1197: "RuntimeException occurred in RequiredModelMBean > "+ > 1198: "while trying to invoke operation " + opName); This one is a bit different. You've collapsed RuntimeOperationsException into the RuntimeException handler, which seems fine since it is a subclass of RuntimeException and both handlers do the same thing. However, I wonder if the original intent of the RuntimeOperationsException handler was for it to have a different message, and perhaps reference RuntimeOperationsException instead of RuntimeException. - PR: https://git.openjdk.java.net/jdk/pull/8161
Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
On Mon, 11 Apr 2022 11:55:33 GMT, Roman Kennke wrote: >> JVMTI heap walking marks objects in order to track which have been visited >> already. In order to do that, it uses bits in the object header. Those are >> the same bits that are also used by some GCs to mark objects (the lowest two >> bits, also used by locking code). Some GCs also use the bits in order to >> indicate 'forwarded' objects, where the upper bits of the header represent >> the forward-pointer. In the case of Shenandoah, it's even more problematic >> because this happens concurrently, even while JVMTI heap walks can >> intercept. So far we carefully worked around that problem, but it becomes >> very problematic in Lilliput, where accesses to the Klass* also requires to >> decode the header, and figure out what bits means what. >> >> In addition to that, marking objects in their header requires that the >> original header gets saved and restored. We only do that for 'interesting' >> headers, that is headers that have a stack-lock, monitor or hash-code. All >> other headers are reset to their default value. This means we are losing >> object's GC age. This is not catastrophic, but nontheless interferes with >> GC. >> >> JFR already has a datastructure called BitSet to support object marking >> without messing with object's headers. We can use that in JVMTI too. >> >> Testing: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [x] serviceability/jvmti >> - [x] vmTestbase/nsk/jvmti > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add some basic tests for ObjectBitSet src/hotspot/share/jfr/leakprofiler/chains/jfrbitset.hpp line 26: > 24: > 25: #ifndef SHARE_JFR_LEAKPROFILER_JFRBITMAP_HPP > 26: #define SHARE_JFR_LEAKPROFILER_JFRBITMAP_HPP This is wrong - it should be BITSET not BITMAP - PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8269537: memset() is called after operator new [v4]
On Mon, 11 Apr 2022 13:50:49 GMT, Leo Korinth wrote: >> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review updates > > I would like to have this change approved, or the approval of removing > tracking the allocation type altogether. I do not like having this pull > request open any more. The current code is not okay. @lkorinth it is not clear what state this PR is in as you have a merge-conflict with mainline that has not been addressed. I suggest updating the PR so it is ready to integrate again and then resume the review. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5387
Withdrawn: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Fri, 8 Apr 2022 22:15:21 GMT, Leonid Mesnik wrote: > The tests serviceability/jvmti/RedefineClasses start failing in loom-repo > with CNFE like "java.lang.NoClassDefFoundError: > jdk/test/lib/compiler/InMemoryJavaCompiler" > > It is not a loom specific bug, it might happen in jdk/jdk also. > > The workaround is to build some classes explicitly. The fix was implemented > in repo-loom in Nov 2021 and there was no such failure after the fix in the > loom repo. > > Also fix inlcudes some text refactoring to make push from loom smaller. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Sat, 9 Apr 2022 00:39:46 GMT, Alex Menkov wrote: >> The tests serviceability/jvmti/RedefineClasses start failing in loom-repo >> with CNFE like "java.lang.NoClassDefFoundError: >> jdk/test/lib/compiler/InMemoryJavaCompiler" >> >> It is not a loom specific bug, it might happen in jdk/jdk also. >> >> The workaround is to build some classes explicitly. The fix was implemented >> in repo-loom in Nov 2021 and there was no such failure after the fix in the >> loom repo. >> >> Also fix inlcudes some text refactoring to make push from loom smaller. > > I added evaluation in Jira @alexmenkov, @dholmes-ora, @AlanBateman Thank you for your comments. I am withdrawing this PR and fix it in loom properly. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Fri, 8 Apr 2022 13:43:39 GMT, Alan Bateman wrote:
> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
> JDK version to target.
>
> We will refresh this PR periodically to pick up changes and fixes from the
> loom repo.
>
> Most of the new mechanisms in the HotSpot VM are disabled by default and
> require running with `--enable-preview` to enable.
>
> The patch has support for x64 and aarch64 on the usual operating systems
> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero
> and some of the other ports. Additional ports can be contributed via PRs
> against the fibers branch in the loom repo.
>
> There are changes in many areas. To reduce notifications/mails, the labels
> have been trimmed down for now to hotspot, serviceability and core-libs.
> We'll add the complete set of labels when the PR is further along.
>
> The changes include a refresh of java.util.concurrent and ForkJoinPool from
> Doug Lea's CVS. These changes will probably be proposed and integrated in
> advance of this PR.
>
> The changes include some non-exposed and low-level infrastructure to support
> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
> make life a bit easier and avoid having to separate VM changes and juggle
> branches at this time.
src/java.base/share/classes/java/io/BufferedReader.java line 101:
> 99: */
> 100: public BufferedReader(Reader in, int sz) {
> 101: Objects.requireNonNull(in);
Not sure if that even matters - but there will be a slight change of behaviour
here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`. Instead of
synchronizing on `in`, the `BufferedReader` will synchronize on `this`.
Now that I think of it - it probably does matter since even if
CAN_USE_INTERNAL_LOCK is true, untrusted subclasses of BufferedReader calling
this constructor might expect the locking to be performed on `in`?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Integrated: 8284687: validate-source failure after JDK-8283710
A trivial copyright fix to solve validate-source failure after JDK-8283710. - Commit messages: - 8284687: validate-source failure after JDK-8283710 Changes: https://git.openjdk.java.net/jdk/pull/8181/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8181&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284687 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8181/head:pull/8181 PR: https://git.openjdk.java.net/jdk/pull/8181
Integrated: 8284687: validate-source failure after JDK-8283710
On Mon, 11 Apr 2022 16:20:02 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix to solve validate-source failure after JDK-8283710. This pull request has now been integrated. Changeset: 470a6684 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/470a66840cda88d3be07f2b7c4c164c3265603e1 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8284687: validate-source failure after JDK-8283710 Reviewed-by: iris - PR: https://git.openjdk.java.net/jdk/pull/8181
Re: Integrated: 8284687: validate-source failure after JDK-8283710
On Mon, 11 Apr 2022 16:20:02 GMT, Daniel D. Daugherty wrote: > A trivial copyright fix to solve validate-source failure after JDK-8283710. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8181
Re: Integrated: 8284687: validate-source failure after JDK-8283710
On Mon, 11 Apr 2022 16:21:34 GMT, Iris Clark wrote: >> A trivial copyright fix to solve validate-source failure after JDK-8283710. > > Marked as reviewed by iris (Reviewer). @irisclark - Thanks for the lightning fast review! - PR: https://git.openjdk.java.net/jdk/pull/8181
Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
On Mon, 11 Apr 2022 11:55:33 GMT, Roman Kennke wrote: >> JVMTI heap walking marks objects in order to track which have been visited >> already. In order to do that, it uses bits in the object header. Those are >> the same bits that are also used by some GCs to mark objects (the lowest two >> bits, also used by locking code). Some GCs also use the bits in order to >> indicate 'forwarded' objects, where the upper bits of the header represent >> the forward-pointer. In the case of Shenandoah, it's even more problematic >> because this happens concurrently, even while JVMTI heap walks can >> intercept. So far we carefully worked around that problem, but it becomes >> very problematic in Lilliput, where accesses to the Klass* also requires to >> decode the header, and figure out what bits means what. >> >> In addition to that, marking objects in their header requires that the >> original header gets saved and restored. We only do that for 'interesting' >> headers, that is headers that have a stack-lock, monitor or hash-code. All >> other headers are reset to their default value. This means we are losing >> object's GC age. This is not catastrophic, but nontheless interferes with >> GC. >> >> JFR already has a datastructure called BitSet to support object marking >> without messing with object's headers. We can use that in JVMTI too. >> >> Testing: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [x] serviceability/jvmti >> - [x] vmTestbase/nsk/jvmti > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add some basic tests for ObjectBitSet Thank you all! GHA are green too, so I'll - PR: https://git.openjdk.java.net/jdk/pull/7964
Integrated: 8283710: JVMTI: Use BitSet for object marking
On Fri, 25 Mar 2022 20:10:32 GMT, Roman Kennke wrote: > JVMTI heap walking marks objects in order to track which have been visited > already. In order to do that, it uses bits in the object header. Those are > the same bits that are also used by some GCs to mark objects (the lowest two > bits, also used by locking code). Some GCs also use the bits in order to > indicate 'forwarded' objects, where the upper bits of the header represent > the forward-pointer. In the case of Shenandoah, it's even more problematic > because this happens concurrently, even while JVMTI heap walks can intercept. > So far we carefully worked around that problem, but it becomes very > problematic in Lilliput, where accesses to the Klass* also requires to decode > the header, and figure out what bits means what. > > In addition to that, marking objects in their header requires that the > original header gets saved and restored. We only do that for 'interesting' > headers, that is headers that have a stack-lock, monitor or hash-code. All > other headers are reset to their default value. This means we are losing > object's GC age. This is not catastrophic, but nontheless interferes with GC. > > JFR already has a datastructure called BitSet to support object marking > without messing with object's headers. We can use that in JVMTI too. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 > - [x] serviceability/jvmti > - [x] vmTestbase/nsk/jvmti This pull request has now been integrated. Changeset: abfd2f98 Author:Roman Kennke URL: https://git.openjdk.java.net/jdk/commit/abfd2f98dcbe3e96efe52b1d66e4c2efb3542955 Stats: 840 lines in 13 files changed: 392 ins; 427 del; 21 mod 8283710: JVMTI: Use BitSet for object marking Reviewed-by: stuefe, coleenp - PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8269537: memset() is called after operator new [v4]
On Wed, 20 Oct 2021 09:36:38 GMT, Leo Korinth wrote: >> The basic problem is that we are relying on undefined behaviour, as >> documented in the code: >> >> // This whole business of passing information from ResourceObj::operator new >> // to the ResourceObj constructor via fields in the "object" is technically >> UB. >> // But it seems to work within the limitations of HotSpot usage (such as no >> // multiple inheritance) with the compilers and compiler options we're using. >> // And it gives some possibly useful checking for misuse of ResourceObj. >> >> >> I am removing the undefined behaviour by passing the type of allocation >> through a thread local variable. >> >> This solution has some advantages: >> 1) it is not UB >> 2) it is simpler and easier to understand >> 3) it uses less memory (I could make it use even less if I made the enum >> `allocation_type` a u8) >> 4) in the *very* unlikely situation that stack memory (or embedded) already >> equals the data calculated from the address of the object, the code will >> also work. >> >> When doing the change, I also updated `allocated_on_stack()` to the new >> name `allocated_on_stack_or_embedded()` which is much harder to misinterpret. >> >> I also disallow to "fake" the memory type by explicitly calling >> `ResourceObj::set_allocation_type`. >> >> This forced me to change two places that is faking the allocation type of an >> embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking >> of the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray` >> can allocate any type of object. My guess is that `GrowableArray` has >> changed behaviour, or maybe that it was hard to understand because the old >> naming of `allocated_on_stack()`. >> >> I have also tried to update the comments. In doing that I not only changed >> the comments for this change, but also for the *incorrect* advice to always >> delete object you allocate with new. >> >> Testing on debug build tier1-3 >> Testing on release build tier1 > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > review updates I would like to have this change approved, or the approval of removing tracking the allocation type altogether. I do not like having this pull request open any more. The current code is not okay. - PR: https://git.openjdk.java.net/jdk/pull/5387
Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
On Mon, 11 Apr 2022 11:55:33 GMT, Roman Kennke wrote: >> JVMTI heap walking marks objects in order to track which have been visited >> already. In order to do that, it uses bits in the object header. Those are >> the same bits that are also used by some GCs to mark objects (the lowest two >> bits, also used by locking code). Some GCs also use the bits in order to >> indicate 'forwarded' objects, where the upper bits of the header represent >> the forward-pointer. In the case of Shenandoah, it's even more problematic >> because this happens concurrently, even while JVMTI heap walks can >> intercept. So far we carefully worked around that problem, but it becomes >> very problematic in Lilliput, where accesses to the Klass* also requires to >> decode the header, and figure out what bits means what. >> >> In addition to that, marking objects in their header requires that the >> original header gets saved and restored. We only do that for 'interesting' >> headers, that is headers that have a stack-lock, monitor or hash-code. All >> other headers are reset to their default value. This means we are losing >> object's GC age. This is not catastrophic, but nontheless interferes with >> GC. >> >> JFR already has a datastructure called BitSet to support object marking >> without messing with object's headers. We can use that in JVMTI too. >> >> Testing: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [x] serviceability/jvmti >> - [x] vmTestbase/nsk/jvmti > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add some basic tests for ObjectBitSet All good - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
On Mon, 11 Apr 2022 11:55:33 GMT, Roman Kennke wrote: >> JVMTI heap walking marks objects in order to track which have been visited >> already. In order to do that, it uses bits in the object header. Those are >> the same bits that are also used by some GCs to mark objects (the lowest two >> bits, also used by locking code). Some GCs also use the bits in order to >> indicate 'forwarded' objects, where the upper bits of the header represent >> the forward-pointer. In the case of Shenandoah, it's even more problematic >> because this happens concurrently, even while JVMTI heap walks can >> intercept. So far we carefully worked around that problem, but it becomes >> very problematic in Lilliput, where accesses to the Klass* also requires to >> decode the header, and figure out what bits means what. >> >> In addition to that, marking objects in their header requires that the >> original header gets saved and restored. We only do that for 'interesting' >> headers, that is headers that have a stack-lock, monitor or hash-code. All >> other headers are reset to their default value. This means we are losing >> object's GC age. This is not catastrophic, but nontheless interferes with >> GC. >> >> JFR already has a datastructure called BitSet to support object marking >> without messing with object's headers. We can use that in JVMTI too. >> >> Testing: >> - [x] tier1 >> - [x] tier2 >> - [x] tier3 >> - [x] serviceability/jvmti >> - [x] vmTestbase/nsk/jvmti > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Add some basic tests for ObjectBitSet I think this looks great and thank you for all your work resolving comments and bringing this in from the lilliput changes you were going to make. This is a big improvement imo and lets us potentially fix other problems in the jvmti tag map implementation. The template instantiation doesn't bother me. There's only 2 and it's not a big code bloat and it's the way the code has been dealing with memflags. Maybe the mechanism should be revisited, like the GC change that Kim pointed to, but I don't see the motivation to make it something different. Maybe a later RFE can propose a different mechanism for memflags in general. We picked this because we thought it was simple. Thank you for your work on this, Roman and your comments Thomas. test/hotspot/gtest/utilities/test_objectBitSet.cpp line 38: > 36: // allocate a fragement for the memory range starting at 0 and mark the > first bit when passing NULL. > 37: // In the absense of any error handling, I am not sure what would > possibly be a reasonable better > 38: // way to do it, though. Thanks for the comment. If it matters someday we'll have the comment.. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7964
RFR: 8284673: Collapse identical catch branches in java.management
Let's take advantage of Java 7 language feature - "Catching Multiple Exception Types". It simplifies code. Reduces duplication. Found by IntelliJ IDEA inspection Identical 'catch' branches in 'try' statement - Commit messages: - [PATCH] Collapse identical catch branches in java.management Changes: https://git.openjdk.java.net/jdk/pull/8161/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8161&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284673 Stats: 170 lines in 10 files changed: 2 ins; 115 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/8161.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8161/head:pull/8161 PR: https://git.openjdk.java.net/jdk/pull/8161
Re: RFR: 8283710: JVMTI: Use BitSet for object marking
On Sat, 9 Apr 2022 06:41:11 GMT, Thomas Stuefe wrote:
>>> On Apr 9\, 2022\, at 2\:44 AM\, Thomas Stuefe \>> openjdk\.java\.net> wrote\:
>>>
>>> On Fri\, 8 Apr 2022 17\:34\:57 GMT\, Roman Kennke \>> openjdk\.org> wrote\:
>>>
Yes\, I get that\. But the fragments and fragment\-table are themselves
inner classes that take a MEMFLAGS template\. I could \(perhaps\) either
use a constexpr MEMFLAGS arg and pass this through\, or do at some point a
switch like\:
\`\`\`
switch \(\_flags\) \{
case mtServiceability\:
\.\.\. new BitMapFragmentTable\\(\)\; break\;
case mtServiceability\:
\.\.\. new BitMapFragmentTable\\(\)\; break\;
default\: ShouldNotReachHere\(\)\;
\}
\`\`\`
Which seems kinda\-ugly but would work \(I think\)\, and avoid making the
outer class template\-ized\.
>>>
>>> I see what you mean\. This MEMFLAGS template parameter is deeply interwoven
>>> into everything\. I\'d just live with the current solution\. It uses
>>> established pattern\, so at least nobody is surprised\.
>>>
>>> I think the basic problem is that CHeapObj itself is a template class\.
>>> Rethinking MEMFLAGS seems worthwhile for a future RFE\. As I wrote\, one
>>> approach could be to make them a property of the current thread\, and
>>> switchable and stackable via a Mark class\. That way\, everything allocated
>>> within a given range of frames would count toward a given category\. No
>>> need to decide on a fine\-granular basis\. No need for templates\. Maybe no
>>> need even to have a MEMFLAGS argument for every allocation\.
>>
>> While working on something else I ran into a similar problem and found a
>> different
>> approach that seemed to work well\. I\?m planning to explore it in the
>> context of
>> CHeapObj\, but haven\?t gotten around to it yet\. I should file an RFE in
>> case someone
>> else is interested\.
>
>> Yes, I get that. But the fragments and fragment-table are themselves inner
>> classes that take a MEMFLAGS template. I could (perhaps) either use a
>> constexpr MEMFLAGS arg and pass this through, or do at some point a switch
>> like:
>>
>> ```
>> switch (_flags) {
>> case mtServiceability:
>> ... new BitMapFragmentTable(); break;
>> case mtServiceability:
>> ... new BitMapFragmentTable(); break;
>> default: ShouldNotReachHere();
>> }
>> ```
>>
>> Which seems kinda-ugly but would work (I think), and avoid making the outer
>> class template-ized.
>
> I see what you mean. This MEMFLAGS template parameter is deeply interwoven
> into everything. I'd just live with the current solution. It uses established
> pattern, so at least nobody is surprised.
>
> I think the basic problem is that CHeapObj itself is a template class.
> Rethinking MEMFLAGS seems worthwhile for a future RFE. As I wrote, one
> approach could be to make them a property of the current thread, and
> switchable and stackable via a Mark class. That way, everything allocated
> within a given range of frames would count toward a given category. No need
> to decide on a fine-granular basis. No need for templates. Maybe no need even
> to have a MEMFLAGS argument for every allocation.
Have we reached a consensus that the current proposal is the way to go? If so,
could you please mark the latest revision as Reviewed (again), @tstuefe and
@coleenp (and whoever else feels like doing so)?
I also added some basic gtests for ObjectBitSet. Please note my remark on the
NULL test case.
-
PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8283710: JVMTI: Use BitSet for object marking [v8]
> JVMTI heap walking marks objects in order to track which have been visited > already. In order to do that, it uses bits in the object header. Those are > the same bits that are also used by some GCs to mark objects (the lowest two > bits, also used by locking code). Some GCs also use the bits in order to > indicate 'forwarded' objects, where the upper bits of the header represent > the forward-pointer. In the case of Shenandoah, it's even more problematic > because this happens concurrently, even while JVMTI heap walks can intercept. > So far we carefully worked around that problem, but it becomes very > problematic in Lilliput, where accesses to the Klass* also requires to decode > the header, and figure out what bits means what. > > In addition to that, marking objects in their header requires that the > original header gets saved and restored. We only do that for 'interesting' > headers, that is headers that have a stack-lock, monitor or hash-code. All > other headers are reset to their default value. This means we are losing > object's GC age. This is not catastrophic, but nontheless interferes with GC. > > JFR already has a datastructure called BitSet to support object marking > without messing with object's headers. We can use that in JVMTI too. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 > - [x] serviceability/jvmti > - [x] vmTestbase/nsk/jvmti Roman Kennke has updated the pull request incrementally with one additional commit since the last revision: Add some basic tests for ObjectBitSet - Changes: - all: https://git.openjdk.java.net/jdk/pull/7964/files - new: https://git.openjdk.java.net/jdk/pull/7964/files/9ea51d78..b2d913b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7964&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7964&range=06-07 Stats: 66 lines in 1 file changed: 66 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7964.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7964/head:pull/7964 PR: https://git.openjdk.java.net/jdk/pull/7964
Re: RFR: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Mon, 11 Apr 2022 06:41:54 GMT, Alan Bateman wrote: >> The tests serviceability/jvmti/RedefineClasses start failing in loom-repo >> with CNFE like "java.lang.NoClassDefFoundError: >> jdk/test/lib/compiler/InMemoryJavaCompiler" >> >> It is not a loom specific bug, it might happen in jdk/jdk also. >> >> The workaround is to build some classes explicitly. The fix was implemented >> in repo-loom in Nov 2021 and there was no such failure after the fix in the >> loom repo. >> >> Also fix inlcudes some text refactoring to make push from loom smaller. > > The issue with RedefineClassTest throwing NoClassDefFoundError: > jdk/test/lib/InMemoryJavaCompiler seems to go back to 2016 at least. We've > had issues in other areas in the past that stemmed from implicit compilation > so I assume this is what is suspected here too. > > In the loom repo, two of the existing RedefineClasss* classes have an > explicit `@compile` because the updated tests needed to be compiled with > `--enable-preview`. I would be surprised if this caused an issue but maybe it > creates an issue with concurrent test execution when tests that depend on > implicit compilation are running at the same time (in another agentvm) but > doing explicit compilation? I wonder if creating a TEST.properties with > `exclusiveAccess.dirs=.` would help this area. > @AlanBateman adding the explicit compile commands to add `--enable-preview` > is exactly what causes the problem. By compiling that individual file first > it also compiled some testlib dependencies but puts the classes in a > different place. When another class is later compiled, the compilation path > includes that destination and so compilation succeeds, but at runtime the > path is different and we get the NCDFE. This is the analysis that Alex did in > a similar issue - see his comment in JBS. Adding `--enable-preview` did not intentionally mean to introduce implicit compilation but it looks like we'll have to re-visit the ordering (as @alexmenkov suggests) in the loom repo. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284616: Implement workaround for CODETOOLS-7901986 in serviceability/jvmti/RedefineClasses
On Mon, 11 Apr 2022 06:41:54 GMT, Alan Bateman wrote: >> The tests serviceability/jvmti/RedefineClasses start failing in loom-repo >> with CNFE like "java.lang.NoClassDefFoundError: >> jdk/test/lib/compiler/InMemoryJavaCompiler" >> >> It is not a loom specific bug, it might happen in jdk/jdk also. >> >> The workaround is to build some classes explicitly. The fix was implemented >> in repo-loom in Nov 2021 and there was no such failure after the fix in the >> loom repo. >> >> Also fix inlcudes some text refactoring to make push from loom smaller. > > The issue with RedefineClassTest throwing NoClassDefFoundError: > jdk/test/lib/InMemoryJavaCompiler seems to go back to 2016 at least. We've > had issues in other areas in the past that stemmed from implicit compilation > so I assume this is what is suspected here too. > > In the loom repo, two of the existing RedefineClasss* classes have an > explicit `@compile` because the updated tests needed to be compiled with > `--enable-preview`. I would be surprised if this caused an issue but maybe it > creates an issue with concurrent test execution when tests that depend on > implicit compilation are running at the same time (in another agentvm) but > doing explicit compilation? I wonder if creating a TEST.properties with > `exclusiveAccess.dirs=.` would help this area. @AlanBateman adding the explicit compile commands to add `--enable-preview` is exactly what causes the problem. By compiling that individual file first it also compiled some testlib dependencies but puts the classes in a different place. When another class is later compiled, the compilation path includes that destination and so compilation succeeds, but at runtime the path is different and we get the NCDFE. This is the analysis that Alex did in a similar issue - see his comment in JBS. - PR: https://git.openjdk.java.net/jdk/pull/8170
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Mon, 11 Apr 2022 07:12:09 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/misc/StructureViolationExceptions.java
>> line 81:
>>
>>> 79: Constructor ctor;
>>> 80: try {
>>> 81: Class exClass =
>>> Class.forName("jdk.incubator.concurrent.StructureViolationException");
>>
>> Should this not be `jdk.internal.misc`? (Also, if this is the case, maybe
>> there's a test missing that could have discovered this...)
>
>> Should this not be `jdk.internal.misc`? (Also, if this is the case, maybe
>> there's a test missing that could have discovered this...)
>
> The exception is in an incubator module, it's just that code in java.base
> can't statically reference it.
Maybe it should use a `MethodHandle` fetched using `IMPL_LOOKUP` instead, in
order to avoid the runtime overhead of going through `CallerSensitive` methods
(`Class.forName` and `Constructor.newInstance`).
It should probably also be cached.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Mon, 11 Apr 2022 07:33:20 GMT, ExE Boss wrote: > Maybe it should use a `MethodHandle` fetched using `IMPL_LOOKUP` instead, in > order to avoid the runtime overhead of going through `CallerSensitive` > methods (`Class.forName` and `Constructor.newInstance`). > > It should probably also be cached. It is cached. It's also a very exceptional case that only arises when the SC API (separate JEP, not here) is used incorrectly. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Sun, 10 Apr 2022 22:14:41 GMT, Magnus Ihse Bursie wrote: > Should this not be `jdk.internal.misc`? (Also, if this is the case, maybe > there's a test missing that could have discovered this...) The exception is in an incubator module, it's just that code in java.base can't statically reference it. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Sun, 10 Apr 2022 00:40:02 GMT, ExE Boss wrote: > This is supposed to point to the `package‑summary.html` Thanks, this link is indeed wrong. Will be fixed when we refresh. - PR: https://git.openjdk.java.net/jdk/pull/8166
