RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused timeout failures when graal is enabled. The fix is to avoid suspending all threads when a breakpoint is reached and then resume just the main thread again. This pattern was used in the test case EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal threads remained suspended and, running with -Xbatch, the main thread waited (with timeout) for completion of compile tasks. The fix was applied to all breakpoints in the test. All explicit suspend calls now apply only to the main test thread and all explicit resume calls apply to all java threads. Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn is reduced from 30s to 10s. - Commit messages: - 8255381: com/sun/jdi/EATests.java should not suspend graal threads Changes: https://git.openjdk.java.net/jdk/pull/1625/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1625&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255381 Stats: 91 lines in 2 files changed: 33 ins; 10 del; 48 mod Patch: https://git.openjdk.java.net/jdk/pull/1625.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1625/head:pull/1625 PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
On Fri, 4 Dec 2020 20:12:11 GMT, Chris Plummer wrote: >> This PR replaces the withdrawn PR #1348. This PR tries to fix the underlying >> problem, rather than fix the tests. >> >> The problem is that a number of JDI tests create objects on the debugger >> side with calls to `newInstance()`. However, on the debugee side, these new >> instances will only be held on to by a `JNIGlobalWeakRef`, which means they >> could be collected at any time, even before `newInstace()` returns. A number >> of JDI tests get spurious `ObjectCollectedException` thrown at them, which >> results in test failures. To make these objects stick around, a call to >> `disableCollection()` is typically needed. >> >> However, as pointer out by @plummercj in >> [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987): >> >>> Going back to the spec, ObjectReference.disableCollection() says: >>> >>> "By default all ObjectReference values returned by JDI may be collected at >>> any time the target VM is running" >>> >>> and >>> >>> "Note that while the target VM is suspended, no garbage collection will >>> occur because all threads are suspended." >>> >>> But no where does is say what is meant by the VM running or being >>> suspended, or how to get it in that state. One might assume that this ties >>> in with VirtualMachine.suspend(), but it says: >>> >>> "Suspends the execution of the application running in this virtual machine. >>> All threads currently running will be suspended." >>> >>> No mention of suspending the VM, but that certainly seems to be what is >>> implied by the method name and also by the loose wording in >>> disableCollection(). >> >> Most of our spuriously failing tests do actually make a call to >> `VirtualMachine.suspend()`, presumably to prevent objects from being garbage >> collected. However, the current implementation of `VirtualMachine.suspend()` >> will only suspend all Java threads. That is not enough to prevent objects >> from being garbage collected. The GC can basically run at any time, and >> there is no relation to whether all Java threads are suspended or not. >> >> However, as suggested by @plummercj, we could emulate the behaviour implied >> by the spec by letting a call to `VirtualMachine.suspend()` also convert all >> existing JDI objects references to be backed by a (strong) `JNIGlobalRef` >> rather than a (weak) `JNIGlobalWeakRef`. That will not prevent the GC from >> running, but it will prevent any object visible to a JDI client from being >> garbage collected. Of course, a call to `VirtualMachine.resume()` would >> convert all references back to being weak again. >> >> This patch introduces the needed functions in `libjdwp` to "pin" and "unpin" >> all objects. These new functions are then used by the underpinnings of >> `VirtualMachine.suspend()` and `VirtualMachine.resume()` to implement the >> behaviour described above. >> >> Note that there are still a few tests that needed adjustments to guard >> against `ObjectCollectionException`. These are: >> - *vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java* - This >> test seems to have been forgotten by >> [JDK-8203174](https://bugs.openjdk.java.net/browse/JDK-8203174), which did a >> similar fix in the other `ArrayType/newinstance` tests. >> - >> *vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java* >> - We just want to allocate as much as we can, so catching an ignoring >> `ObjectCollectedException` seems reasonable here. >> - *vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java* - We still want to >> prevent `TestClassLoader` from being unloaded to avoid invalidating code >> locations. >> - >> *vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java* >> - This test keeps the VM suspended, and then expects objects to be garbage >> collected, which they now won't. >> >> Testing: >> - More than 50 iterations of the `vmTestbase/nsk/jdi` and >> `vmTestbase/nsk/jdwp` test suites, using various GC, both in mach5 and >> locally. > > test/hotspot/jtreg/vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java > line 194: > >> 192: debuggee.resume(); >> 193: checkDebugeeAnswer_instances(className, >> baseInstances); >> 194: debuggee.suspend(); > > Before the changes in this PR, what was triggering the (expected) collection > of the objects? @plummercj Nothing was explicitly triggering collection of these objects. However, the test is explicitly checking the number of objects "reachable for the purposes of garbage collection" in `checkDebugeeAnswer_instances()`. The tests sets up a breakpoint (with SUSPEND_ALL), which suspends the VM. Then it creates a number of new instances and expects these to be weakly reachable. However, with this change, suspending the VM will make all objects "reachable for the purposes of garbage collection". S
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
On Mon, 7 Dec 2020 05:14:36 GMT, David Holmes wrote:
>> Another options is to save away the weakref in the node when strengthening.
>> This would benefit `ObjectReference.disableCollection()` also, since it
>> would no longer need to deal with a potential OOM. However, I'm not so sure
>> it's actually worth doing. Trying to keep the debug session alive will
>> having allocation errors is probably a fools errand.
>
> I agree a fatal error here seems excessive. Simply maintaining the strong ref
> seems reasonable.
I was trying to mimic what we already do in `strengthenNode()`, i.e. it's a
fatal error if we can't create a JNI ref. Here:
strongRef = JNI_FUNC_PTR(env,NewGlobalRef)(env, node->ref);
/*
* NewGlobalRef on a weak ref will return NULL if the weak
* reference has been collected or if out of memory.
* It never throws OOM.
* We need to distinguish those two occurrences.
*/
if ((strongRef == NULL) && !isSameObject(env, node->ref, NULL)) {
EXIT_ERROR(AGENT_ERROR_NULL_POINTER,"NewGlobalRef");
}
So it seems appropriate to do the same thing if we fail to create a JNI weak
ref. Also, as @plummercj mentioned, if we can't create a JNI ref, continuing
the debug session seems rather pointless as we're about to go down anyway (the
next allocation in the JVM will be fatal).
-
PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
On Mon, 7 Dec 2020 05:10:34 GMT, David Holmes wrote: >> This PR replaces the withdrawn PR #1348. This PR tries to fix the underlying >> problem, rather than fix the tests. >> >> The problem is that a number of JDI tests create objects on the debugger >> side with calls to `newInstance()`. However, on the debugee side, these new >> instances will only be held on to by a `JNIGlobalWeakRef`, which means they >> could be collected at any time, even before `newInstace()` returns. A number >> of JDI tests get spurious `ObjectCollectedException` thrown at them, which >> results in test failures. To make these objects stick around, a call to >> `disableCollection()` is typically needed. >> >> However, as pointer out by @plummercj in >> [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987): >> >>> Going back to the spec, ObjectReference.disableCollection() says: >>> >>> "By default all ObjectReference values returned by JDI may be collected at >>> any time the target VM is running" >>> >>> and >>> >>> "Note that while the target VM is suspended, no garbage collection will >>> occur because all threads are suspended." >>> >>> But no where does is say what is meant by the VM running or being >>> suspended, or how to get it in that state. One might assume that this ties >>> in with VirtualMachine.suspend(), but it says: >>> >>> "Suspends the execution of the application running in this virtual machine. >>> All threads currently running will be suspended." >>> >>> No mention of suspending the VM, but that certainly seems to be what is >>> implied by the method name and also by the loose wording in >>> disableCollection(). >> >> Most of our spuriously failing tests do actually make a call to >> `VirtualMachine.suspend()`, presumably to prevent objects from being garbage >> collected. However, the current implementation of `VirtualMachine.suspend()` >> will only suspend all Java threads. That is not enough to prevent objects >> from being garbage collected. The GC can basically run at any time, and >> there is no relation to whether all Java threads are suspended or not. >> >> However, as suggested by @plummercj, we could emulate the behaviour implied >> by the spec by letting a call to `VirtualMachine.suspend()` also convert all >> existing JDI objects references to be backed by a (strong) `JNIGlobalRef` >> rather than a (weak) `JNIGlobalWeakRef`. That will not prevent the GC from >> running, but it will prevent any object visible to a JDI client from being >> garbage collected. Of course, a call to `VirtualMachine.resume()` would >> convert all references back to being weak again. >> >> This patch introduces the needed functions in `libjdwp` to "pin" and "unpin" >> all objects. These new functions are then used by the underpinnings of >> `VirtualMachine.suspend()` and `VirtualMachine.resume()` to implement the >> behaviour described above. >> >> Note that there are still a few tests that needed adjustments to guard >> against `ObjectCollectionException`. These are: >> - *vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java* - This >> test seems to have been forgotten by >> [JDK-8203174](https://bugs.openjdk.java.net/browse/JDK-8203174), which did a >> similar fix in the other `ArrayType/newinstance` tests. >> - >> *vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java* >> - We just want to allocate as much as we can, so catching an ignoring >> `ObjectCollectedException` seems reasonable here. >> - *vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java* - We still want to >> prevent `TestClassLoader` from being unloaded to avoid invalidating code >> locations. >> - >> *vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java* >> - This test keeps the VM suspended, and then expects objects to be garbage >> collected, which they now won't. >> >> Testing: >> - More than 50 iterations of the `vmTestbase/nsk/jdi` and >> `vmTestbase/nsk/jdwp` test suites, using various GC, both in mach5 and >> locally. > > src/jdk.jdwp.agent/share/native/libjdwp/commonRef.c line 586: > >> 584: jobject strongRef; >> 585: >> 586: strongRef = strengthenNode(env, node); > > This can just be one line. I was actually trying to carefully to follow the coding style currently used in this file/library. If you have a quick look at this file you'll see the pattern above in multiple places, where as combined declaration+assignment style isn't used. So while I personally agree about this style question, I also think following the style already present in a file has precedence over introducing a new style. Don't you agree? - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
> This PR replaces the withdrawn PR #1348. This PR tries to fix the underlying > problem, rather than fix the tests. > > The problem is that a number of JDI tests create objects on the debugger side > with calls to `newInstance()`. However, on the debugee side, these new > instances will only be held on to by a `JNIGlobalWeakRef`, which means they > could be collected at any time, even before `newInstace()` returns. A number > of JDI tests get spurious `ObjectCollectedException` thrown at them, which > results in test failures. To make these objects stick around, a call to > `disableCollection()` is typically needed. > > However, as pointer out by @plummercj in > [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987): > >> Going back to the spec, ObjectReference.disableCollection() says: >> >> "By default all ObjectReference values returned by JDI may be collected at >> any time the target VM is running" >> >> and >> >> "Note that while the target VM is suspended, no garbage collection will >> occur because all threads are suspended." >> >> But no where does is say what is meant by the VM running or being suspended, >> or how to get it in that state. One might assume that this ties in with >> VirtualMachine.suspend(), but it says: >> >> "Suspends the execution of the application running in this virtual machine. >> All threads currently running will be suspended." >> >> No mention of suspending the VM, but that certainly seems to be what is >> implied by the method name and also by the loose wording in >> disableCollection(). > > Most of our spuriously failing tests do actually make a call to > `VirtualMachine.suspend()`, presumably to prevent objects from being garbage > collected. However, the current implementation of `VirtualMachine.suspend()` > will only suspend all Java threads. That is not enough to prevent objects > from being garbage collected. The GC can basically run at any time, and there > is no relation to whether all Java threads are suspended or not. > > However, as suggested by @plummercj, we could emulate the behaviour implied > by the spec by letting a call to `VirtualMachine.suspend()` also convert all > existing JDI objects references to be backed by a (strong) `JNIGlobalRef` > rather than a (weak) `JNIGlobalWeakRef`. That will not prevent the GC from > running, but it will prevent any object visible to a JDI client from being > garbage collected. Of course, a call to `VirtualMachine.resume()` would > convert all references back to being weak again. > > This patch introduces the needed functions in `libjdwp` to "pin" and "unpin" > all objects. These new functions are then used by the underpinnings of > `VirtualMachine.suspend()` and `VirtualMachine.resume()` to implement the > behaviour described above. > > Note that there are still a few tests that needed adjustments to guard > against `ObjectCollectionException`. These are: > - *vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java* - This test > seems to have been forgotten by > [JDK-8203174](https://bugs.openjdk.java.net/browse/JDK-8203174), which did a > similar fix in the other `ArrayType/newinstance` tests. > - > *vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java* > - We just want to allocate as much as we can, so catching an ignoring > `ObjectCollectedException` seems reasonable here. > - *vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java* - We still want to prevent > `TestClassLoader` from being unloaded to avoid invalidating code locations. > - > *vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java* - > This test keeps the VM suspended, and then expects objects to be garbage > collected, which they now won't. > > Testing: > - More than 50 iterations of the `vmTestbase/nsk/jdi` and > `vmTestbase/nsk/jdwp` test suites, using various GC, both in mach5 and > locally. Per Liden has updated the pull request incrementally with one additional commit since the last revision: Add comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/1595/files - new: https://git.openjdk.java.net/jdk/pull/1595/files/5b32d271..8fe1e52d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1595&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1595&range=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1595.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1595/head:pull/1595 PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 07:41:46 GMT, Chris Plummer wrote:
>> That was something I pointed out in the pre-review, and it has been
>> addressed in `commonRef_pinAll/unpinAll`:
>>
>> `568 if (gdata->pinAllCount == 1) {`
>> `618 if (gdata->pinAllCount == 0) {`
>
>> Okay. I would not have handled it at that level, but would have had
> pinAll/unpinAll operate unconditionally, but the calls to those methods
> being conditional based on the suspendAllCount.
>>
>>David
>
> Well, that's assuming `pinAll()` will only ever be used by by `suspendAll()`.
> One could imaging a future use, such as if
> `VirtualMachine.disableCollection()` were ever to be added.
I was also thinking `pinAll()/unpinAll()` should stand on their own, and not
implicitly depend/rely on `suspendAllCount`. As @plummercj says, one could
imagine we want to use these functions in other contexts in the future.
-
PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
On 7/12/2020 5:44 pm, Chris Plummer wrote:
On Mon, 7 Dec 2020 06:27:20 GMT, Chris Plummer wrote:
src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 1560:
1558: * garbage collected while the VM is suspended.
1559: */
1560: commonRef_pinAll();
Can we have multiple VM.suspend calls? The suspendAllCount seems to suggest that. In
which case shouldn't we only pin on the 0->1 transition, and only unpin on the
1->0 transition?
That was something I pointed out in the pre-review, and it has been addressed
in `commonRef_pinAll/unpinAll`:
`568 if (gdata->pinAllCount == 1) {`
`618 if (gdata->pinAllCount == 0) {`
Okay. I would not have handled it at that level, but would have had
pinAll/unpinAll operate unconditionally, but the calls to those methods
being conditional based on the suspendAllCount.
David
Well, that's assuming `pinAll()` will only ever be used by by `suspendAll()`.
One could imaging a future use, such as if `VirtualMachine.disableCollection()`
were ever to be added.
Not really. I consider pinAll should pin-all as the name implies. The
question of when to pin should be handled by the caller of pinAll. If
there were ever to be a second reason to pinAll then you would have to
decide what semantics that has: does it maintain a count, or is it like
thread suspension.
David
-
PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
On 7/12/2020 9:12 pm, Per Liden wrote: On Mon, 7 Dec 2020 05:10:34 GMT, David Holmes wrote: 584: jobject strongRef; 585: 586: strongRef = strengthenNode(env, node); This can just be one line. I was actually trying to carefully to follow the coding style currently used in this file/library. If you have a quick look at this file you'll see the pattern above in multiple places, where as combined declaration+assignment style isn't used. So while I personally agree about this style question, I also think following the style already present in a file has precedence over introducing a new style. Don't you agree? This file uses an archaic C-style, so while I agree it would be inappropriate to over modernise the new code, this particular example stuck out because even in archaic C there is no reason to split this onto two lines. I didn't go looking to see if this mimicked existing code. :) Keep it or change it as you see fit. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 06:04:36 GMT, David Holmes wrote: >> Per Liden has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add comment > > Overall seems okay. Some comments on tests as I think the existing test logic > is quite confused in places. > > Thanks, > David > Not really. I consider pinAll should pin-all as the name implies. The > question of when to pin should be handled by the caller of pinAll. If there > were ever to be a second reason to pinAll then you would have to decide what > semantics that has: does it maintain a count, or is it like thread suspension. I would say that would not be in spirit of how the rest of this library is designed, with regards to nesting of calls. For example, `pin()/unpin()`, `suspend()/resume()`, `createNode()/deleteNode()`, etc. All these functions supports nesting, so they might just up/down a counter, instead of doing exactly what their name implies. The new `pinAll()/unpinAll()` follow the same model, which, to me, feels like the natural thing to do here. - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - Diagnose synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. For the core libraries parts looks ok. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - Diagnose synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. The hotspot changes look good. In a future change, could you add additional classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. Currently, it only tests primitive classes. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Mon, 7 Dec 2020 15:50:55 GMT, Harold Seigel wrote: >> Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). >> >> Development has been broken into 5 tasks, each with its own JBS issue: >> - Deprecate wrapper class constructors for removal (rriggs) >> - Revise "value-based class" & apply to wrappers (dlsmith) >> - Define & apply annotation jdk.internal.ValueBased (rriggs) >> - Add 'lint' warning for @ValueBased classes (sadayapalam) >> - Diagnose synchronization on @ValueBased classes (lfoltan) >> >> All changes have been previously reviewed and integrated into the [`jep390` >> branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` >> repository. See the subtasks of the 5 JBS issues for these changes, >> including discussion and links to reviews. (Reviewers: mchung, dlsmith, >> jlaskey, rriggs, lfoltan, fparain, hseigel.) >> >> CSRs have also been completed or are nearly complete: >> >> - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for >> wrapper class constructor deprecation >> - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for >> revisions to "value-based class" >> - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new >> `javac` lint warnings >> >> Here's an overview of the files changed: >> >> - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to >> an instance of a class tagged with `jdk.internal.ValueBased`. This enhances >> previous work that produced such diagnostics for the primitive wrapper >> classes. >> >> - `src/java.base/share/classes/java/lang`: deprecating for removal the >> wrapper class constructors; revising the definition of "value-based class" >> in `ValueBased.html` and the description used by linking classes; applying >> "value-based class" to the primitive wrapper classes; marking value-based >> classes with `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/java/lang/constant`: no longer designating >> these classes as "value-based", since they rely heavily on field inheritance. >> >> - `src/java.base/share/classes/java/time`: revising the description used to >> link to `ValueBased.html`; marking value-based classes with >> `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/java/util`: revising the description used to >> link to `ValueBased.html`; marking value-based classes with >> `@jdk.internal.ValueBased`. >> >> - `src/java.base/share/classes/jdk/internal`: define the >> `@jdk.internal.ValueBased` annotation. >> >> - `src/java.management.rmi`: removing uses of wrapper class constructors. >> >> - `src/java.xml`: removing uses of wrapper class constructors. >> >> - `src/jdk.compiler`: implementing the `synchronization` lint category, >> which reports attempts to synchronize on classes and interfaces annotated >> with `@jdk.internal.ValueBased`. >> >> - `src/jdk.incubator.foreign`: revising the description used to link to >> `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a >> special module export, which was not deemed necessary for an incubating API.) >> >> - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and >> synchronization warnings in tests >> >> - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics >> >> - `test`: testing new `javac` and HotSpot behavior; removing usages of >> deprecated wrapper class constructors from tests, or suppressing deprecation >> warnings; revising the description used to link to `ValueBased.html`. > > The hotspot changes look good. In a future change, could you add additional > classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. > Currently, it only tests primitive classes. @hseigel thank you for the review. I have created https://bugs.openjdk.java.net/browse/JDK-8257836 as an RFE to address additional testing. Lois - PR: https://git.openjdk.java.net/jdk/pull/1636
RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
This has been implemented for cgroups v1 with [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was lacking some tooling support for cgroups v2. With podman 2.2.0 release this could now be implemented (and tested). The idea is the same as for the cgroups v1 fix. If we've got no swap limit capabilities, return the memory limit only. Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, thus, doesn't have `getMemoryAndSwapFailCount()` and `getMemoryAndSwapMaxUsage()`. Testing: - [x] submit testing - [x] container tests on cgroups v2 with swapaccount=0. - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2. Thoughts? - Commit messages: - 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems Changes: https://git.openjdk.java.net/jdk/pull/1672/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1672&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253797 Stats: 45 lines in 2 files changed: 24 ins; 1 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/1672.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1672/head:pull/1672 PR: https://git.openjdk.java.net/jdk/pull/1672
Re: RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()
On Tue, 17 Nov 2020 12:53:48 GMT, Claes Redestad wrote: > This moves the mirroring of vmSymbols in ciSymbols to a separate file, > ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the > ciSymbol namespace). In a few places code is moved from .hpp to .cpp to > facilitate this. > > With PCH disabled, this reduces total includes from 276949 to 276332 Marked as reviewed by kvn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1256
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 10:44:56 GMT, Per Liden wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java >> line 194: >> >>> 192: debuggee.resume(); >>> 193: checkDebugeeAnswer_instances(className, >>> baseInstances); >>> 194: debuggee.suspend(); >> >> Before the changes in this PR, what was triggering the (expected) collection >> of the objects? > > @plummercj Nothing was explicitly triggering collection of these objects. > However, the test is explicitly checking the number of objects "reachable for > the purposes of garbage collection" in `checkDebugeeAnswer_instances()`. The > tests sets up a breakpoint (with SUSPEND_ALL), which suspends the VM. Then it > creates a number of new instances and expects these to be weakly reachable. > However, with this change, suspending the VM will make all objects "reachable > for the purposes of garbage collection". So, to let the test continue to > create objects which are weakly reachable we need to first resume the VM, > create the new instances, and then suspend it again. > > @dholmes-ora I have no idea why these tests are so different. The VM suspend > is implicit in the breakpoint in this test, which is set up using SUSPEND_ALL. Ok, I understand now. `ReferenceType.instances()` only counts objects "reachable for the purposes of garbage collection". This change in behavior does concern me a little bit. I think the expectation is that the instances created by `ClassType.newInstance()` will not show up in this count unless `disableCollection()` is called, even when under a "suspend all". Clearly that's the expectation of this test, so the question is whether or not it is a reasonable expectation. Note that `ClassType.newInstance()` says nothing about the state of the returned object w.r.t. GC. It makes no mention of the need to call `disableCollection()` before resuming the VM, so I guess this gives us some wiggle room. However, the argument against the object being strongly reachable is comes from user asking the question "who has the strong reference that makes it strongly reachable?". It's not obvious to the user why there is a strong reference, and why it seemingly goes a way once `VM.resumeAll()` is called. I still think overall this is the right approach (baring a better approach being presented), but we may need to include some spec clarifications, and be prepared for some push back if this breaks anything. - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255381: com/sun/jdi/EATests.java should not suspend graal threads
On Fri, 4 Dec 2020 15:30:15 GMT, Richard Reingruber wrote:
> This fixes a bug in the test test/jdk/com/sun/jdi/EATests.java that caused
> timeout failures when graal is enabled.
>
> The fix is to avoid suspending all threads when a breakpoint is reached and
> then resume
> just the main thread again. This pattern was used in the test case
> EAMaterializeLocalAtObjectPollReturnReturn. It caused timeouts because graal
> threads remained suspended and, running with -Xbatch, the main thread waited
> (with timeout) for completion of compile tasks.
> The fix was applied to all breakpoints in the test. All explicit suspend
> calls now apply only
> to the main test thread and all explicit resume calls apply to all java
> threads.
>
> Testing: duration of the test case EAMaterializeLocalAtObjectPollReturnReturn
> is
> reduced from 30s to 10s.
test/jdk/com/sun/jdi/EATests.java line 1274:
> 1272: o = getLocalRef(env.targetMainThread.frame(0),
> XYVAL_NAME, "xy");
> 1273: } catch (Exception e) {
> 1274: msg("The local variable xy is out of scope because we
> suspended at the wrong bci. Resume and try again! (" + (++retryCount) + ")");
Please move the increment of retryCount to before the msg() call for clarify.
test/jdk/com/sun/jdi/EATests.java line 1275:
> 1273: } catch (Exception e) {
> 1274: msg("The local variable xy is out of scope because we
> suspended at the wrong bci. Resume and try again! (" + (++retryCount) + ")");
> 1275: env.vm().resume();
You are calling `VM.resume()` in a loop, yet you are suspending using
`ThreadReference.suspend()`. Although the it looks like this will work, it
seems that calling `ThreadReference.resume()` would be more appropriate.
-
PR: https://git.openjdk.java.net/jdk/pull/1625
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 11:22:30 GMT, Per Liden wrote: >> This PR replaces the withdrawn PR #1348. This PR tries to fix the underlying >> problem, rather than fix the tests. >> >> The problem is that a number of JDI tests create objects on the debugger >> side with calls to `newInstance()`. However, on the debugee side, these new >> instances will only be held on to by a `JNIGlobalWeakRef`, which means they >> could be collected at any time, even before `newInstace()` returns. A number >> of JDI tests get spurious `ObjectCollectedException` thrown at them, which >> results in test failures. To make these objects stick around, a call to >> `disableCollection()` is typically needed. >> >> However, as pointer out by @plummercj in >> [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987): >> >>> Going back to the spec, ObjectReference.disableCollection() says: >>> >>> "By default all ObjectReference values returned by JDI may be collected at >>> any time the target VM is running" >>> >>> and >>> >>> "Note that while the target VM is suspended, no garbage collection will >>> occur because all threads are suspended." >>> >>> But no where does is say what is meant by the VM running or being >>> suspended, or how to get it in that state. One might assume that this ties >>> in with VirtualMachine.suspend(), but it says: >>> >>> "Suspends the execution of the application running in this virtual machine. >>> All threads currently running will be suspended." >>> >>> No mention of suspending the VM, but that certainly seems to be what is >>> implied by the method name and also by the loose wording in >>> disableCollection(). >> >> Most of our spuriously failing tests do actually make a call to >> `VirtualMachine.suspend()`, presumably to prevent objects from being garbage >> collected. However, the current implementation of `VirtualMachine.suspend()` >> will only suspend all Java threads. That is not enough to prevent objects >> from being garbage collected. The GC can basically run at any time, and >> there is no relation to whether all Java threads are suspended or not. >> >> However, as suggested by @plummercj, we could emulate the behaviour implied >> by the spec by letting a call to `VirtualMachine.suspend()` also convert all >> existing JDI objects references to be backed by a (strong) `JNIGlobalRef` >> rather than a (weak) `JNIGlobalWeakRef`. That will not prevent the GC from >> running, but it will prevent any object visible to a JDI client from being >> garbage collected. Of course, a call to `VirtualMachine.resume()` would >> convert all references back to being weak again. >> >> This patch introduces the needed functions in `libjdwp` to "pin" and "unpin" >> all objects. These new functions are then used by the underpinnings of >> `VirtualMachine.suspend()` and `VirtualMachine.resume()` to implement the >> behaviour described above. >> >> Note that there are still a few tests that needed adjustments to guard >> against `ObjectCollectionException`. These are: >> - *vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java* - This >> test seems to have been forgotten by >> [JDK-8203174](https://bugs.openjdk.java.net/browse/JDK-8203174), which did a >> similar fix in the other `ArrayType/newinstance` tests. >> - >> *vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java* >> - We just want to allocate as much as we can, so catching an ignoring >> `ObjectCollectedException` seems reasonable here. >> - *vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java* - We still want to >> prevent `TestClassLoader` from being unloaded to avoid invalidating code >> locations. >> - >> *vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java* >> - This test keeps the VM suspended, and then expects objects to be garbage >> collected, which they now won't. >> >> Testing: >> - More than 50 iterations of the `vmTestbase/nsk/jdi` and >> `vmTestbase/nsk/jdwp` test suites, using various GC, both in mach5 and >> locally. > > Per Liden has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment I still have some reservations about the logic in some of the tests now (ie using disableCollection whilst the VM is suspended and reenabling also whilst suspended) but the logic was unclear in the first place. If necessary follow up cleanup issues could be filed here. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 10:57:08 GMT, Per Liden wrote:
>> I agree a fatal error here seems excessive. Simply maintaining the strong
>> ref seems reasonable.
>
> I was trying to mimic what we already do in `strengthenNode()`, i.e. it's a
> fatal error if we can't create a JNI ref. Here:
>
> strongRef = JNI_FUNC_PTR(env,NewGlobalRef)(env, node->ref);
> /*
> * NewGlobalRef on a weak ref will return NULL if the weak
> * reference has been collected or if out of memory.
> * It never throws OOM.
> * We need to distinguish those two occurrences.
> */
> if ((strongRef == NULL) && !isSameObject(env, node->ref, NULL)) {
> EXIT_ERROR(AGENT_ERROR_NULL_POINTER,"NewGlobalRef");
> }
>
> So it seems appropriate to do the same thing if we fail to create a JNI weak
> ref. Also, as @plummercj mentioned, if we can't create a JNI ref, continuing
> the debug session seems rather pointless as we're about to go down anyway
> (the next allocation in the JVM will be fatal).
Okay.
-
PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 20:30:07 GMT, Chris Plummer wrote: >> @plummercj Nothing was explicitly triggering collection of these objects. >> However, the test is explicitly checking the number of objects "reachable >> for the purposes of garbage collection" in `checkDebugeeAnswer_instances()`. >> The tests sets up a breakpoint (with SUSPEND_ALL), which suspends the VM. >> Then it creates a number of new instances and expects these to be weakly >> reachable. However, with this change, suspending the VM will make all >> objects "reachable for the purposes of garbage collection". So, to let the >> test continue to create objects which are weakly reachable we need to first >> resume the VM, create the new instances, and then suspend it again. >> >> @dholmes-ora I have no idea why these tests are so different. The VM suspend >> is implicit in the breakpoint in this test, which is set up using >> SUSPEND_ALL. > > Ok, I understand now. `ReferenceType.instances()` only counts objects > "reachable for the purposes of garbage collection". This change in behavior > does concern me a little bit. I think the expectation is that the instances > created by `ClassType.newInstance()` will not show up in this count unless > `disableCollection()` is called, even when under a "suspend all". Clearly > that's the expectation of this test, so the question is whether or not it is > a reasonable expectation. > > Note that `ClassType.newInstance()` says nothing about the state of the > returned object w.r.t. GC. It makes no mention of the need to call > `disableCollection()` before resuming the VM, so I guess this gives us some > wiggle room. However, the argument against the object being strongly > reachable is comes from user asking the question "who has the strong > reference that makes it strongly reachable?". It's not obvious to the user > why there is a strong reference, and why it seemingly goes a way once > `VM.resumeAll()` is called. > > I still think overall this is the right approach (baring a better approach > being presented), but we may need to include some spec clarifications, and be > prepared for some push back if this breaks anything. I don't follow your reasoning here Chris. All ObjectReferences can be GC'd at any time unless GC has been disallowed. So a reference create via newInstance is no different to any other reference. If it is currently reachable then instances() should return it. Are you treating "reachable for the purposes of garbage collection" as-if it said "strongly reachable"? It doesn't so I think you are reading too much into this. I think there is a lot of flexibility in this API in terms of what it may return regarding weak references. - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException [v2]
On Mon, 7 Dec 2020 22:14:28 GMT, David Holmes wrote: >> Ok, I understand now. `ReferenceType.instances()` only counts objects >> "reachable for the purposes of garbage collection". This change in behavior >> does concern me a little bit. I think the expectation is that the instances >> created by `ClassType.newInstance()` will not show up in this count unless >> `disableCollection()` is called, even when under a "suspend all". Clearly >> that's the expectation of this test, so the question is whether or not it is >> a reasonable expectation. >> >> Note that `ClassType.newInstance()` says nothing about the state of the >> returned object w.r.t. GC. It makes no mention of the need to call >> `disableCollection()` before resuming the VM, so I guess this gives us some >> wiggle room. However, the argument against the object being strongly >> reachable is comes from user asking the question "who has the strong >> reference that makes it strongly reachable?". It's not obvious to the user >> why there is a strong reference, and why it seemingly goes a way once >> `VM.resumeAll()` is called. >> >> I still think overall this is the right approach (baring a better approach >> being presented), but we may need to include some spec clarifications, and >> be prepared for some push back if this breaks anything. > > I don't follow your reasoning here Chris. All ObjectReferences can be GC'd at > any time unless GC has been disallowed. So a reference create via newInstance > is no different to any other reference. If it is currently reachable then > instances() should return it. Are you treating "reachable for the purposes of > garbage collection" as-if it said "strongly reachable"? It doesn't so I think > you are reading too much into this. I think there is a lot of flexibility in > this API in terms of what it may return regarding weak references. I read "reachable for the purposes of garbage collection" as not including objects reachable only via weak reference. So if the only reference to an object is a weak reference, which is normally what you have after calling `ClassType.newInstance()`, then the object is not considered reachable. At the very least, his is how `ReferenceType.instances()` is implemented, and is based on JVMTI [FollowReferences](https://docs.oracle.com/en/java/javase/14/docs/specs/jvmti.html#FollowReferences)(). So given that, the expectation would be that an object returned `ClassType.newInstance()` would not be counted by `ReferenceType.instances()` unless something is done to add a strong reference to the object, such as calling `ObjectReference.disableCollection()`. Now with Per's changes a strong reference is also created with doing a VM.suspend(). The test doesn't expect this behavior, and it's understandable why. - PR: https://git.openjdk.java.net/jdk/pull/1595
Re: RFR: 8256424: Move ciSymbol::symbol_name() to ciSymbols::symbol_name()
On Tue, 17 Nov 2020 12:53:48 GMT, Claes Redestad wrote: > This moves the mirroring of vmSymbols in ciSymbols to a separate file, > ciSymbols.hpp, to reduce includes throughout hotspot (and clean up the > ciSymbol namespace). In a few places code is moved from .hpp to .cpp to > facilitate this. > > With PCH disabled, this reduces total includes from 276949 to 276332 Changes requested by iklam (Reviewer). src/hotspot/share/ci/bcEscapeAnalyzer.hpp line 102: > 100: void compute_escape_info(); > 101: vmIntrinsicID known_intrinsic(); > 102: void compute_escape_for_intrinsic(vmIntrinsicID iid); I think the use of vmIntrinsics::ID in bcEscapeAnalyzer.cpp should also be changed to vmIntrinsicID for consistency, even though they are the same type. (The same for other hpp files such as ciMethod.hpp) src/hotspot/share/opto/library_call.cpp line 27: > 25: #include "precompiled.hpp" > 26: #include "asm/macroAssembler.hpp" > 27: #include "ci/ciSymbols.hpp" This file doesn't seem to use the exports from ciSymbols.hpp. - PR: https://git.openjdk.java.net/jdk/pull/1256
