Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]

[Per Liden]

On Tue, 10 Aug 2021 08:59:59 GMT, Per Liden  wrote:

>> While fixing JDK-8270626 it was discovered that the C2 intrinsic for 
>> `Reference.refersTo()` is not used as often as one would think. Instead C2 
>> often prefers using the native implementation, which is much slower which 
>> defeats the purpose of having an intrinsic in the first place.
>> 
>> The problem arise from having `refersTo0()` be virtual, native and 
>> @IntrinsicCandidate. This can be worked around by introducing an virtual 
>> method layer and so that `refersTo0()` can be made `final`. That's what this 
>> patch does, by adding a virtual `refersToImpl()` which in turn calls the now 
>> final `refersTo0()`.
>> 
>> It should be noted that `Object.clone()` and `Object.hashCode()` are also 
>> virtual, native and @IntrinsicCandidate and these methods get special 
>> treatment by C2 to get the intrinsic generated more optimally. We might want 
>> to do a similar optimization for `Reference.refersTo0()`. In that case, it 
>> is suggested that such an improvement could come later and be handled as a 
>> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track 
>> that.
>> 
>> Testing:
>> I found this hard to test without instrumenting Hotspot to tell me that the 
>> intrinsic was called instead of the native method. So, for that reason 
>> testing was ad-hoc, using an instrumented Hotspot in combination with the 
>> test 
>> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) 
>> that initially uncovered the problem. See JDK-8270626 for additional 
>> information.
>
> Per Liden has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Private implies final

Thanks all for reviewing!

-

PR: https://git.openjdk.java.net/jdk/pull/5052

Integrated: 8271862: C2 intrinsic for Reference.refersTo() is often not used

[Per Liden]

On Mon, 9 Aug 2021 13:13:33 GMT, Per Liden  wrote:

> While fixing JDK-8270626 it was discovered that the C2 intrinsic for 
> `Reference.refersTo()` is not used as often as one would think. Instead C2 
> often prefers using the native implementation, which is much slower which 
> defeats the purpose of having an intrinsic in the first place.
> 
> The problem arise from having `refersTo0()` be virtual, native and 
> @IntrinsicCandidate. This can be worked around by introducing an virtual 
> method layer and so that `refersTo0()` can be made `final`. That's what this 
> patch does, by adding a virtual `refersToImpl()` which in turn calls the now 
> final `refersTo0()`.
> 
> It should be noted that `Object.clone()` and `Object.hashCode()` are also 
> virtual, native and @IntrinsicCandidate and these methods get special 
> treatment by C2 to get the intrinsic generated more optimally. We might want 
> to do a similar optimization for `Reference.refersTo0()`. In that case, it is 
> suggested that such an improvement could come later and be handled as a 
> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track 
> that.
> 
> Testing:
> I found this hard to test without instrumenting Hotspot to tell me that the 
> intrinsic was called instead of the native method. So, for that reason 
> testing was ad-hoc, using an instrumented Hotspot in combination with the 
> test 
> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) 
> that initially uncovered the problem. See JDK-8270626 for additional 
> information.

This pull request has now been integrated.

Changeset: 3f723ca4
Author:Per Liden 
URL:   
https://git.openjdk.java.net/jdk/commit/3f723ca4577b9cffeb6153ee386edd75f1dfb1c6
Stats: 14 lines in 2 files changed: 11 ins; 0 del; 3 mod

8271862: C2 intrinsic for Reference.refersTo() is often not used

Reviewed-by: kbarrett, mchung

-

PR: https://git.openjdk.java.net/jdk/pull/5052

Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]

[Per Liden]

On Mon, 9 Aug 2021 19:59:21 GMT, Vladimir Ivanov  wrote:

>> Per Liden has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Private implies final
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 379:
> 
>> 377: 
>> 378: @IntrinsicCandidate
>> 379: private native final boolean refersTo0(Object o);
> 
> No need to have it both `private` and `final`.  Just `private` should be 
> enough.

Good point. Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/5052

Re: RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used [v2]

[Per Liden]

> While fixing JDK-8270626 it was discovered that the C2 intrinsic for 
> `Reference.refersTo()` is not used as often as one would think. Instead C2 
> often prefers using the native implementation, which is much slower which 
> defeats the purpose of having an intrinsic in the first place.
> 
> The problem arise from having `refersTo0()` be virtual, native and 
> @IntrinsicCandidate. This can be worked around by introducing an virtual 
> method layer and so that `refersTo0()` can be made `final`. That's what this 
> patch does, by adding a virtual `refersToImpl()` which in turn calls the now 
> final `refersTo0()`.
> 
> It should be noted that `Object.clone()` and `Object.hashCode()` are also 
> virtual, native and @IntrinsicCandidate and these methods get special 
> treatment by C2 to get the intrinsic generated more optimally. We might want 
> to do a similar optimization for `Reference.refersTo0()`. In that case, it is 
> suggested that such an improvement could come later and be handled as a 
> separate enhancement, and @kimbarrett has already filed JDK-8272140 to track 
> that.
> 
> Testing:
> I found this hard to test without instrumenting Hotspot to tell me that the 
> intrinsic was called instead of the native method. So, for that reason 
> testing was ad-hoc, using an instrumented Hotspot in combination with the 
> test 
> (`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) 
> that initially uncovered the problem. See JDK-8270626 for additional 
> information.

Per Liden has updated the pull request incrementally with one additional commit 
since the last revision:

  Private implies final

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5052/files
  - new: https://git.openjdk.java.net/jdk/pull/5052/files/66743f76..3bc3d5e5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5052&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5052&range=00-01

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5052.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5052/head:pull/5052

PR: https://git.openjdk.java.net/jdk/pull/5052

RFR: 8271862: C2 intrinsic for Reference.refersTo() is often not used

[Per Liden]

While fixing JDK-8270626 it was discovered that the C2 intrinsic for 
`Reference.refersTo()` is not used as often as one would think. Instead C2 
often prefers using the native implementation, which is much slower which 
defeats the purpose of having an intrinsic in the first place.

The problem arise from having `refersTo0()` be virtual, native and 
@IntrinsicCandidate. This can be worked around by introducing an virtual method 
layer and so that `refersTo0()` can be made `final`. That's what this patch 
does, by adding a virtual `refersToImpl()` which in turn calls the now final 
`refersTo0()`.

It should be noted that `Object.clone()` and `Object.hashCode()` are also 
virtual, native and @IntrinsicCandidate and these methods get special treatment 
by C2 to get the intrinsic generated more optimally. We might want to do a 
similar optimization for `Reference.refersTo0()`. In that case, it is suggested 
that such an improvement could come later and be handled as a separate 
enhancement, and @kimbarrett has already filed JDK-8272140 to track that.

Testing:
I found this hard to test without instrumenting Hotspot to tell me that the 
intrinsic was called instead of the native method. So, for that reason testing 
was ad-hoc, using an instrumented Hotspot in combination with the test 
(`vmTestbase/gc/gctests/PhantomReference/phantom002/TestDescription.java`) that 
initially uncovered the problem. See JDK-8270626 for additional information.

-

Commit messages:
 - 8271862: C2 intrinsic for Reference.refersTo() is often not used

Changes: https://git.openjdk.java.net/jdk/pull/5052/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5052&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8271862
  Stats: 14 lines in 2 files changed: 11 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5052.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5052/head:pull/5052

PR: https://git.openjdk.java.net/jdk/pull/5052

RFR: 8265136: ZGC: Expose GarbageCollectorMXBeans for both pauses and cycles

[Per Liden]

JDK-8240679 introduced a change where the information exposed via the 
GarbageCollectorMXBean went from being related to the GC pauses to instead be 
related to the GC cycles. This helped provide more accurate heap usage 
information. However, some users have noticed that that you no longer get 
timing information related to the GC pauses, only related to the GC cycle.

Shenandoah has opted for having two distinct memory managers to provide timing 
information about both pauses and cycles. To align how concurent GCs report 
this information, ZGC should probably do the same.

Testing:
 * Tier1-7 with ZGC
 * Manual testing with relevant jtreg tests

-

Commit messages:
 - 8265136: ZGC: Expose GarbageCollectorMXBeans for both pauses and cycles

Changes: https://git.openjdk.java.net/jdk/pull/3483/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3483&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265136
  Stats: 179 lines in 9 files changed: 107 ins; 25 del; 47 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3483.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3483/head:pull/3483

PR: https://git.openjdk.java.net/jdk/pull/3483

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]

[Per Liden]

On Mon, 25 Jan 2021 13:23:27 GMT, Magnus Ihse Bursie  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> Changes requested by ihse (Reviewer).

In `make/autoconf/jvm-features.m4` I notice that you haven't enabled ZGC for 
macos/aarch64. Is that just an oversight, or is there a reason for that?

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo

[Per Liden]

On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov  wrote:

> JDK-8188055 added the function Reference.refersTo. For performance, the 
> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 
> should be intrinsified by C2.
> 
> Initial patch was prepared by @fisk.
> 
> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
> 
> Ran new test with Shenandoah. Found only one issue. As result I disable  
> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. 
> Someone from Shenandoah team have to test changes if that is enough.

ZGC changes look good!

-

Marked as reviewed by pliden (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1425

Re: RFR: 8256999: Add C2 intrinsic for Reference.refersTo and PhantomReference::refersTo

[Per Liden]

On Wed, 25 Nov 2020 03:31:36 GMT, Vladimir Kozlov  wrote:

> JDK-8188055 added the function Reference.refersTo. For performance, the 
> supporting native methods Reference.refersTo0 and PhantomReference.refersTo0 
> should be intrinsified by C2.
> 
> Initial patch was prepared by @fisk.
> 
> Tested hs-tier1-4. Added new compiler tests to test intrinsics.
> 
> Ran new test with Shenandoah. Found only one issue. As result I disable  
> PhantomReference::refersTo intrinsic for COOP+ Shenandoah combination. 
> Someone from Shenandoah team have to test changes if that is enough.

src/hotspot/share/opto/library_call.cpp line 5525:

> 5523: Node* LibraryCallKit::load_field_from_object(Node* fromObj, const char* 
> fieldName, const char* fieldTypeString,
> 5524:  DecoratorSet decorators = 
> IN_HEAP, bool is_exact = false, bool is_static = false,
> 5525:  ciInstanceKlass* fromKls = 
> NULL) {

It looks like the `is_exact` argument here can be removed, as all call-sites 
use the default value, which is `false`, and the only use of it in the function 
is this assert, which will never fail.
assert(!is_exact || tinst->klass_is_exact(), "klass not exact");

-

PR: https://git.openjdk.java.net/jdk/pull/1425

Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification [v2]

[Per Liden]

On Tue, 24 Nov 2020 03:05:19 GMT, Kim Barrett  wrote:

>> Please review this change to Reference.clear() to address several issues.
>> 
>> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent
>> field to null may extend the lifetime of the referent value.
>> 
>> (JDK-8240696) For GCs with concurrent reference processing, clearing the
>> referent field during reference processing may discard the expected
>> notification.
>> 
>> Both of these are addressed by introducing a private native helper function
>> for clearing the referent, rather than using an ordinary in-Java field
>> assignment. Tests have been added for both of these issues. This required
>> adding a new breakpoint in reference processing for ZGC.
>> 
>> Of course, finalization adds some complexity to the problem.  We deal with
>> that by having FinalReference override clear.  The implementation is
>> provided by a new package-private method in Reference.  (There are a number
>> of alternatives, all of them clumsy; finalization is annoying that way.)
>> 
>> While dealing with FinalReference clearing it was noted that the recent
>> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not
>> updated to call the new Reference.getInactive(), instead still calling get()
>> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the
>> assertion for inactive FinalReference added by JDK-8256370 used the wrong
>> test.
>> 
>> Rather than tracking down and changing all get() and clear() calls on final
>> references and changing them to use getInactive and a new similar clear
>> function, I've changed FinalReference to override get and clear, which call
>> the helper functions in Reference. I've also renamed getInactive to be more
>> explanatory and less convenient to call directly, and similarly named the
>> helper for clear. This means that get/clear should never be called on an
>> active FinalReference. That's already never done, and would have problems
>> if it were.
>> 
>> Testing:
>> mach5 tier1-6
>> Local (linux-x64) tier1 using Shenandoah.
>> New TestReferenceClearDuringMarking fails for G1 without these changes.
>> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these 
>> changes.
>
> Kim Barrett has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - new tests need ref in oldgen too
>  - remove obsolete comment about races with clear and enqueue

Looks good!

-

Marked as reviewed by pliden (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1376

Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification

[Per Liden]

On Mon, 23 Nov 2020 15:00:08 GMT, Roman Kennke  wrote:

>> Please review this change to Reference.clear() to address several issues.
>> 
>> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent
>> field to null may extend the lifetime of the referent value.
>> 
>> (JDK-8240696) For GCs with concurrent reference processing, clearing the
>> referent field during reference processing may discard the expected
>> notification.
>> 
>> Both of these are addressed by introducing a private native helper function
>> for clearing the referent, rather than using an ordinary in-Java field
>> assignment. Tests have been added for both of these issues. This required
>> adding a new breakpoint in reference processing for ZGC.
>> 
>> Of course, finalization adds some complexity to the problem.  We deal with
>> that by having FinalReference override clear.  The implementation is
>> provided by a new package-private method in Reference.  (There are a number
>> of alternatives, all of them clumsy; finalization is annoying that way.)
>> 
>> While dealing with FinalReference clearing it was noted that the recent
>> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not
>> updated to call the new Reference.getInactive(), instead still calling get()
>> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the
>> assertion for inactive FinalReference added by JDK-8256370 used the wrong
>> test.
>> 
>> Rather than tracking down and changing all get() and clear() calls on final
>> references and changing them to use getInactive and a new similar clear
>> function, I've changed FinalReference to override get and clear, which call
>> the helper functions in Reference. I've also renamed getInactive to be more
>> explanatory and less convenient to call directly, and similarly named the
>> helper for clear. This means that get/clear should never be called on an
>> active FinalReference. That's already never done, and would have problems
>> if it were.
>> 
>> Testing:
>> mach5 tier1-6
>> Local (linux-x64) tier1 using Shenandoah.
>> New TestReferenceClearDuringMarking fails for G1 without these changes.
>> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these 
>> changes.
>
> test/hotspot/jtreg/gc/TestReferenceClearDuringReferenceProcessing.java line 
> 28:
> 
>> 26: /* @test
>> 27:  * @bug 8256517
>> 28:  * @requires vm.gc.Z
> 
> Please add | vm.gc.Shenandoah here.

Note that for this test to be useful, the GC needs to support concurrent GC 
breakpoints, which Shenandoah doesn't do.

-

PR: https://git.openjdk.java.net/jdk/pull/1376

Re: RFR: 8256517: (ref) Reference.clear during reference processing may lose notification

[Per Liden]

On Mon, 23 Nov 2020 01:43:39 GMT, Kim Barrett  wrote:

> Please review this change to Reference.clear() to address several issues.
> 
> (JDK-8240696) For GCs using a SATB barrier, simply assigning the referent
> field to null may extend the lifetime of the referent value.
> 
> (JDK-8240696) For GCs with concurrent reference processing, clearing the
> referent field during reference processing may discard the expected
> notification.
> 
> Both of these are addressed by introducing a private native helper function
> for clearing the referent, rather than using an ordinary in-Java field
> assignment. Tests have been added for both of these issues. This required
> adding a new breakpoint in reference processing for ZGC.
> 
> Of course, finalization adds some complexity to the problem.  We deal with
> that by having FinalReference override clear.  The implementation is
> provided by a new package-private method in Reference.  (There are a number
> of alternatives, all of them clumsy; finalization is annoying that way.)
> 
> While dealing with FinalReference clearing it was noted that the recent
> JDK-8256106 and JDK-8256370 have some problems. FinalizerHistogram was not
> updated to call the new Reference.getInactive(), instead still calling get()
> on FinalReferences, with the JDK-8256106 problems. Fixing that showed the
> assertion for inactive FinalReference added by JDK-8256370 used the wrong
> test.
> 
> Rather than tracking down and changing all get() and clear() calls on final
> references and changing them to use getInactive and a new similar clear
> function, I've changed FinalReference to override get and clear, which call
> the helper functions in Reference. I've also renamed getInactive to be more
> explanatory and less convenient to call directly, and similarly named the
> helper for clear. This means that get/clear should never be called on an
> active FinalReference. That's already never done, and would have problems
> if it were.
> 
> Testing:
> mach5 tier1-6
> Local (linux-x64) tier1 using Shenandoah.
> New TestReferenceClearDuringMarking fails for G1 without these changes.
> New TestReferenceClearDuringReferenceProcessing fails for ZGC without these 
> changes.

Looks good. Just want to request that you also remove the following comment in 
zReferenceProcessor.cpp, as it's no longer true.

--- a/src/hotspot/share/gc/z/zReferenceProcessor.cpp
+++ b/src/hotspot/share/gc/z/zReferenceProcessor.cpp
@@ -184,12 +184,6 @@ bool ZReferenceProcessor::should_discover(oop reference, 
ReferenceType type) con
 }
 
 bool ZReferenceProcessor::should_drop(oop reference, ReferenceType type) const 
{
-  // This check is racing with a call to Reference.clear() from the 
application.
-  // If the application clears the reference after this check it will still end
-  // up on the pending list, and there's nothing we can do about that without
-  // changing the Reference.clear() API. This check is also racing with a call
-  // to Reference.enqueue() from the application, which is unproblematic, since
-  // the application wants the reference to be enqueued anyway.
   const oop referent = reference_referent(reference);
   if (referent == NULL) {
 // Reference has been cleared, by a call to Reference.enqueue()

-

Changes requested by pliden (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1376

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v3]

[Per Liden]

On Wed, 14 Oct 2020 04:27:34 GMT, Kim Barrett  wrote:

>> Finally returning to this review that was started in April 2020.  I've
>> recast it as a github PR.  I think the security concern raised by Gil
>> has been adequately answered.
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
>> 
>> Please review a new function: java.lang.ref.Reference.refersTo.
>> 
>> This function is needed to test the referent of a Reference object without
>> artificially extending the lifetime of the referent object, as may happen
>> when calling Reference.get.  Some garbage collectors require extending the
>> lifetime of a weak referent when accessed, in order to maintain collector
>> invariants.  Lifetime extension may occur with any collector when the
>> Reference is a SoftReference, as calling get indicates recent access.  This
>> new function also allows testing the referent of a PhantomReference, which
>> can't be accessed by calling get.
>> 
>> The new function uses native methods whose implementations are in the VM so
>> they can use the Access API.  It is the intent that these methods will be
>> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
>> implemented yet.  Bear that in mind before rushing off to change existing
>> uses of Reference.get.
>> 
>> There are two native methods involved, one in Reference and an override in
>> PhantomReference, both package private in java.lang.ref. The reason for this
>> split is to simplify the intrinsification. This is a change from the version
>> from April 2020; that version had a single native method in Reference,
>> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
>> However, adding support for that category in the compilers adds significant
>> implementation effort and complexity. Splitting avoids that complexity.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) verified the new test passes with various garbage 
>> collectors.
>
> Kim Barrett has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - simplify test
>  - cleanup nits from Mandy
>  - use Object instead of TestObject

Marked as reviewed by pliden (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v3]

[Per Liden]

On Thu, 8 Oct 2020 14:00:11 GMT, Roger Riggs  wrote:

>> Can you add a unit test in `test/jdk/java/lang/ref` that serves as a basic 
>> API test for this new `refersTo` API without
>> depending the WhiteBox API? test/hotspot/jtreg/gc` test serves as a more 
>> white-boxed type and comprehensive test.
>
> Hi Kim,
> 
> At what point would the @IntrinsicCandidate annotation be added to the
> refersTo0 methods?
> it would be useful documentation even if it is not needed for the mechanism.
> 
> Thanks for the explanation, Roger
> 
> 
> On 10/8/20 3:58 AM, mlbridge[bot] wrote:
>>
>> /Mailing list message from Kim Barrett 
>> on hotspot-gc-dev :/
>>
>> On Oct 5, 2020, at 9:15 PM, Roger Riggs > openjdk.java.net> wrote:
>> src/java.base/share/classes/java/lang/ref/PhantomReference.java
>> line 66:
>>
>> 64: @override
>> 
>> 
>> 65: native final boolean refersTo0(Object o);
>> 66:
>>
>> How does the existence of this method help future intrinsification?
>> If it was called somewhere it would be clearer.
>> Perhaps a comment now or later would help explain its function.
>>
>> public final refersTo(T o) calls refersTo0(o)
>>
>> We have package private:
>> native boolean Reference::refersTo0(Object)
>> final native boolean PhantomReference::refersTo0(Object)
>>
>> The reason for this split has to do with details in the VM support, in
>> particular C2 intrinsification.
>>
>> For the native method support we don't need this split. The original
>> version from back in April just had a single native method in
>> Reference. (It
>> did have native refersTo0, but that was my not realizing native methods
>> could have a parameterized type that would get type-erased automatically;
>> see response to Mandy.) That native method performed a runtime check
>> on the
>> ReferenceType of the reference's klass to determine what to do. (See
>> below.)
>>
>> Phantom references need to be treated differently from stronger "weak"
>> reference types, because phantom references are weaker than
>> finalization, so
>> might not be cleared when a stronger reference to the same object is
>> cleared. For collectors with STW reference processing this doesn't make
>> much difference (the referent is either cleared or not), but making this
>> distinction correctly is necessary for concurrent reference processing.
>>
>> The Access API that provides the interface to the GC has support for
>> "unknown" referent accesses that perform a runtime lookup. This is
>> supported in C++ code, but not in the various Java code processors
>> (interpreter and compilers). We didn't previously need to support that
>> case
>> for Java because the only place where it mattered was accessing the
>> referent
>> of a PhantomReference, and that is blocked by PhantomReference::get that
>> always returns null.
>>
>> For refersTo the intent is to have the interpreter and C1 use the native
>> method, but have C2 have special knowledge for it. We could add
>> support for
>> the "unknown" reference type to C2, but that's a substantial amount of
>> work,
>> and only useful for this one place. Or we can take the same approach
>> as for
>> get(), i.e. have a second method on PhantomReference so that calls
>> that can
>> select the appropriate method at compile time (usually the case) can have
>> distinct intrinsics for the two cases.
>>
>> By having these intrinsifiable native methods be package private we
>> can have
>> the public refersTo API function be final, while also preventing any
>> further
>> overrides by classes not in the same package.
>>
>> I'll try to come up with some commentary.
>>
>> —
>> You are receiving this because you commented.
>> Reply to this email directly, view it on GitHub
>> ,
>> or unsubscribe
>> .
>>

> In the places where expectNotCleared is used, the expected value is 
> unavailable; the associated testObjectN variable
> has been nulled.

Ah, true.

-

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8188055: (ref) Add Reference.refersTo predicate

[Per Liden]

Hi Kim,

On 4/8/20 2:25 AM, Kim Barrett wrote:

[Note review on both core-libs and hotspot-gc-dev lists; try not to lose
either when replying.]

Please review a new function: java.lang.ref.Reference.refersTo.


Nice!



This function is needed to test the referent of a Reference object
without artificially extending the lifetime of the referent object, as
may happen when calling Reference.get.  Some garbage collectors
require extending the lifetime of a weak referent when accessed, in
order to maintain collector invariants.  Lifetime extension may occur
with any collector when the Reference is a SoftReference, as calling
get indicates recent access.  This new function also allows testing
the referent of a PhantomReference, which can't be accessed by calling
get.

The new function uses a native method whose implementation is in the
VM so it can use the Access API.  It is the intent that this function
will be intrinsified by optimizing compilers like C2 or graal, but
that hasn't been implemented yet.  Bear that in mind before rushing
off to change existing uses of Reference.get.


Do we have an enhancement filed for this? We should make it very clear 
that such an intrinsic is not allowed to safepoint between loading the 
referent and comparing it.




CR:
https://bugs.openjdk.java.net/browse/JDK-8188055
https://bugs.openjdk.java.net/browse/JDK-8241029 (CSR)

Webrev:
https://cr.openjdk.java.net/~kbarrett/8188055/open.04/


The patch looks good to me, just two minor comments:


java/lang/ref/Reference.java


 349  * @param   obj is the object to compare with the referenced 
object, or

 350  *  {@code null}.

May I suggest "referenced object" -> "referent" to keep the nomenclature 
intact.



gc/TestReferenceRefersTo.java
-

 208 long timeout = 1000;
 209 while (true) {
 210 Reference ref = 
queue.remove(timeout);

 211 if (ref == null) {
 212 break;
 213 } else if (ref == testPhantom1) {
 214 testPhantom1 = null;
 215 } else if (ref == testWeak2) {
 216 testWeak2 = null;
 217 } else if (ref == testWeak3) {
 218 testWeak3 = null;
 219 } else if (ref == testWeak4) {
 220 testWeak4 = null;
 221 } else {
 222 fail("unexpected reference in queue");
 223 }
 224 }

That timeout look unnecessarily short, and I imagine it could cause 
intermittent failures on really slow machines. I would suggest we make 
that more like 60 seconds, alternatively skip the timeout all together 
and let the whole test timeout if the expected references don't appear 
in the queue.


cheers,
Per



Testing:
mach5 tier1

Locally (linux-x64) verified the new test passes with various garbage
collectors.

Re: SoftReference incorrect javadoc?

[Per Liden]

Hi Michael,

On 04/17/2019 03:59 AM, Michael Pollmeier wrote:

Hi Per,

While testing different JVMs I realized that it's fixed in openjdk 11,
e.g. openjdk version "11.0.1" 2018-10-16 LTS (zulu build), maybe by this
commit: https://hg.openjdk.java.net/jdk/jdk/rev/6464882498b5


That bug only affected ZGC (which was introduced in 11), so it didn't 
change the behavior of any other GC.



That's great to know, but is it worth updating the javadocs of older
versions?

To reproduce it I attached a simple SoftRefTest.java to easily reproduce
it. It allocates (only) softly referenced objects that occupy some heap,
occasionally printing counts (instantiated, finalized, free heap).


Thanks for the test. The problem with the test is that you have a 
finalize() function on Instance. This means the object will be kept 
around until it has been finalized. So even if the SoftReference 
referring to it is cleared, the memory will not be freed until the 
object also has been finalized. In your test, you're simply creating too 
many objects, and the Finalizer thread is simply unable to keep up 
finalizing them at the same pace as they are created, so the heap fills up.


I adjusted your test a bit, and removed the use of finalize(). You can 
run this test forever without running into OOMEs. By having the same 
thread that creates the objects also poll the reference queue, the test 
will automatically be throttled. If SoftReferences weren't cleared as 
they should, this test would OOME pretty fast.


import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.SoftReference;
import java.util.HashMap;

public class SoftRefTest2 {
  public static void main(String[] args) throws Exception {
final ReferenceQueue queue = new ReferenceQueue<>();
final HashMap> instances = new 
HashMap<>();

long createdCount = 0;
long clearedCount = 0;

for (;;) {
  SoftReference softref = new SoftReference(new 
Instance(), queue);

  instances.put(softref, softref);
  if (++createdCount % 10 == 0) {
System.out.println(createdCount + " instances created; free=" + 
Runtime.getRuntime().freeMemory() / 1024 / 1024 + "M");

  }

  // Drain queue
  Reference ref;
  while ((ref = queue.poll()) != null) {
  instances.remove(ref);
  if (++clearedCount % 10 == 0) {
  System.out.println(clearedCount + " instances cleared");
  }
  }
}
  }

  static class Instance {
static int finalizedCount = 0;
String[] occupySomeHeap = new String[50];
  }
}


cheers,
Per



Usage:
javac SoftRefTest.java
java -Xms256m -Xmx256m SoftRefTest

Output:
10 instances created; free=212M
20 instances created; free=181M
30 instances created; free=152M
40 instances created; free=121M
50 instances created; free=93M
60 instances created; free=61M
70 instances created; free=33M
Exception in thread "main" java.lang.OutOfMemoryError: Java heap space
 at Instance.(SoftRefTest.java:27)
 at SoftRefTest.main(SoftRefTest.java:12)

Interpretation:
It doesn't free any of the only softly referenced objects, resulting in
an OutOfMemoryError, contradicting the 'guarantee' in the javadoc.

Workaround: if you additionally configure
`-XX:SoftRefLRUPolicyMSPerMB=0`, it finalizes them and doesn't run out
of memory.

Tested with:
openjdk version "1.8.0_212"
java version "1.8.0_144" (oracle)
openjdk version "9.0.4.1" (zulu build)
openjdk version "10.0.2" 2018-07-17  (zulu build)

Cheers
Michael


On 16/04/19 6:27 pm, Per Liden wrote:

Hi Michael,

On 4/16/19 4:19 AM, David Holmes wrote:

Hi Michael,

Re-directing to core-libs-dev and hotspot-gc-dev.

Thanks,
David

On 16/04/2019 12:14 pm, Michael Pollmeier wrote:

Quoting
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/SoftReference.html




All soft references to softly-reachable objects are guaranteed to have

been cleared before the virtual machine throws an OutOfMemoryError

That statement was true when soft references were first introduced in
java 1.2, but from java 1.3.1 the jvm property
`-XX:SoftRefLRUPolicyMSPerMB` was introduced.


That statement is still true. When the GC gets into a situation where it
is having trouble satisfying an allocation, then SoftRefLRUPolicyMSPerMB
will be ignored and all soft references will be cleared, before the GC
gives up and throws an OOME.


It defaults to 1000 (milliseconds), meaning that if there’s only 10MB
available heap, the garbage collector will free references that have
been used more than 10s ago. I.e. everything else (including young
softly reachable objects) will *not* be freed, leading to an
OutOfMemoryError, contradicting the above quoted 'guarantee'.

That's also the behaviour I observed on various JREs. Would you 

Re: SoftReference incorrect javadoc?

[Per Liden]

Hi Michael,

On 4/16/19 4:19 AM, David Holmes wrote:

Hi Michael,

Re-directing to core-libs-dev and hotspot-gc-dev.

Thanks,
David

On 16/04/2019 12:14 pm, Michael Pollmeier wrote:

Quoting
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/SoftReference.html 





All soft references to softly-reachable objects are guaranteed to have

been cleared before the virtual machine throws an OutOfMemoryError

That statement was true when soft references were first introduced in
java 1.2, but from java 1.3.1 the jvm property
`-XX:SoftRefLRUPolicyMSPerMB` was introduced.


That statement is still true. When the GC gets into a situation where it 
is having trouble satisfying an allocation, then SoftRefLRUPolicyMSPerMB 
will be ignored and all soft references will be cleared, before the GC 
gives up and throws an OOME.



It defaults to 1000 (milliseconds), meaning that if there’s only 10MB
available heap, the garbage collector will free references that have
been used more than 10s ago. I.e. everything else (including young
softly reachable objects) will *not* be freed, leading to an
OutOfMemoryError, contradicting the above quoted 'guarantee'.

That's also the behaviour I observed on various JREs. Would you agree,
i.e. should I propose an updated doc?


Could you elaborate on what kind of test you did to come to this 
conclusion? Preferably provide a re-producer. In OOME situations, if a 
SoftReference isn't cleared, it is typically because you have 
unknowingly made it strongly reachable.


cheers,
Per

Re: RFR: 8188066: (ref) Examine the reachability of JNI WeakGlobalRef and interaction with phantom refs

[Per Liden]

On 03/13/2019 09:29 PM, Kim Barrett wrote:

On Mar 13, 2019, at 4:07 PM, Kim Barrett  wrote:

Please review this change to the JNI specification.  The specified
behavior of Weak Global References, and in particular their
relationship to Java PhantomReference, is being updated to account
for a change to PhantomReference by JDK-8071507.

CR:
https://bugs.openjdk.java.net/browse/JDK-8188066

CSR:
https://bugs.openjdk.java.net/browse/JDK-8220617

Changes:
http://cr.openjdk.java.net/~kbarrett/8188066/jni_specdiff_rev1.html
  specdiff for the changes.  In addition to updating the section
  describing Weak Global References, the description of NewGlobalRef
  is updated, as well as the copyright footer.

http://cr.openjdk.java.net/~kbarrett/8188066/weak_global_refs_rev1.md
  Copies of the before and after text for the JNI Weak Global
  References section; easier to read than diffs or specdiff.


Clear and unambiguous, nice!

Reviewed.

/Per

Re: WeakReference with null referent

[Per Liden]

On 2018-07-09 20:49, mandy chung wrote:



On 7/9/18 11:31 AM, Zheka Kozlov wrote:

It is possible to create a WeakReference/SoftReference/PhantomReference
with a null value in which case the Reference will never be enqueued. 
This

is quite obvious (since null cannot be weakly/softly/phantom reachable).
But I think it's worth being mentioned in the JavaDoc. What do you think?



Alternatively, the constructor should require non-null referent and 
throws NPE if null.


I created https://bugs.openjdk.java.net/browse/JDK-8206933 to track this.


It's not completely obvious to me that throwing NPE or otherwise 
blocking this is the right thing to do. Sure, creating a Reference with 
a null referent seems pretty useless, but it's also very similar to 
creating a Reference and immediately calling its clear() method, which 
is perfectly valid (and equally useless).


Are you saying we should block this because we can easily detect this 
particular case/misuse, as opposed to the immediately-called-clear case? 
Or is there some other rationale?


cheers,
Per

Re: RFR: Small cleanups in java.lang.ref

[Per Liden]

On 05/17/2018 10:03 PM, mark.reinh...@oracle.com wrote:

2018/5/16 18:31:15 -0700, marti...@google.com:

I've been confused by NULL vs null for years.


That’s because `NULL` in ReferenceQueue.java refers to the singleton
object `ReferenceQueue.NULL`, not the null value.  See the long comment
at the top of Reference.java for an explanation.

To improve this you could change mentions of `NULL` in the comments to
`ReferenceQueue.NULL`, but changing them to `null` would be incorrect.


The comments that were changed in the proposed patch refer to the 
Reference.next and Reference.discovered fields, not Reference.queue. So 
"null" should be correct there.


/Per




8203327: Small cleanups in java.lang.ref
http://cr.openjdk.java.net/~martin/webrevs/jdk/Reference-cleanup/
https://bugs.openjdk.java.net/browse/JDK-8203327


The other changes look fine.

- Mark

Re: RFR(s): 8191859: SoftReference clock/timestamp should be declared volatile

[Per Liden]

Hi Martin,

On 2017-11-24 23:56, Martin Buchholz wrote:

These fields have been this way for a very long time, probably
intentionally non-volatile.
Is there an observable problem being solved here?


Sorry for not being more clear about that. Having them non-volatile can 
lead to the timestamp going backwards (e.g. in C2, if a clock load is 
cached over a safepoint), which in turn can lead to incorrect decision 
being made by the GC SoftRef policy about whether the referent should be 
cleared or not. While an incorrect decision is sub-optimal, it's not 
catastrophic. Worst case is that we clear a soft ref per-maturely.


However, after some discussions here we realized that the suggested fix 
isn't enough to prevent this from happening, e.g. when the interpreter 
is running without thread local safepoint polling. A robust fix for this 
would likely need a CAS to prevent the timestamp from going backwards.


For these reasons, I'm withdrawing my suggested change for now. I need 
to think more about the implication of a robust fix, or if we should 
just live with this issue.


cheers,
Per


I see that out-of-thin-air longs are theoretically possible, but very
rare in practice on current platforms.
If all you're trying to do is to ensure atomicity and that the
reads/writes don't get optimized away by the jit, then consider using
"opaque" VarHandles instead.

I was surprised there was no bug subcomponent for java.lang.ref

On Fri, Nov 24, 2017 at 5:13 AM, Per Liden mailto:per.li...@oracle.com>> wrote:

The clock and timestamp fields in SoftReference should be declared
volatile. These fields are read or updated by the VM, (possibly)
concurrently with the execution of Java threads.

To be more specific, the timestamp field is read by the GC during
reference discovery, e.g. during G1/CMS concurrent mark. The clock
is updated by the GC, typically after reference processing has
completed.

Webrev: http://cr.openjdk.java.net/~pliden/8191859/webrev.0/
<http://cr.openjdk.java.net/~pliden/8191859/webrev.0/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8191859
<https://bugs.openjdk.java.net/browse/JDK-8191859>

/Per

RFR(s): 8191859: SoftReference clock/timestamp should be declared volatile

[Per Liden]

The clock and timestamp fields in SoftReference should be declared 
volatile. These fields are read or updated by the VM, (possibly) 
concurrently with the execution of Java threads.


To be more specific, the timestamp field is read by the GC during 
reference discovery, e.g. during G1/CMS concurrent mark. The clock is 
updated by the GC, typically after reference processing has completed.


Webrev: http://cr.openjdk.java.net/~pliden/8191859/webrev.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8191859

/Per

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Still looks good.

cheers,
Per

On 2016-08-23 23:09, Kim Barrett wrote:

Mandy asked for some additional comments to make life easier for
future readers.  I've also stopped pretending the description for
waitForReferenceProcessing is javadoc, since this function is private
and only accessible via SharedSecrets or reflection that avoids access
control.

Changes only in the jdk tree:

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.05/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.05.inc/

Unchanged hotspot webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04.inc/

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Kim,

On 2016-08-09 03:25, Kim Barrett wrote:

On Aug 8, 2016, at 8:16 AM, Per Liden  wrote:

Hi Kim,

On 2016-08-01 20:47, Kim Barrett wrote:

This updated webrev addresses concerns about the performance of the VM
API used by the reference processor thread in the original webrev.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03/
 http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03.incr/
 http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03.incr/


Overall I think you managed to hit a good balance here, keeping the VM API 
straight forward without loosing flexibility. Having the ReferenceHandler doing 
the actual work seems sound and avoids some complexity, which I like.


Thanks for looking at this.


I have one suggestion though, regarding CheckReferencePendingList(). While reviewing I 
found that I had to check several times what its return value actually meant, the 
"check" part of the name doesn't quite reveal that.
Further, it seems to me that the waiting path of this function has fairly 
little in common with the non-waiting path, e.g. it always returns true. So, to 
make both the naming and implementation more clear I'd like to suggest that we 
split this into two separate functions, HasReferencePendingList() and 
WaitForReferencePendingList(), like this:


I was thinking about splitting things way, and ended up not doing so
for no good reason I can think of. And it does seem clearer that way,
so...

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04.inc/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04.inc/


Thanks for fixing, looks good.

cheers,
Per




Other than this I think the patch looks good.



The originally offered approach was an attempt to minimize changes.  I
was trying to be as similar to the existing code as possible, while
moving part of it into the VM, where we have the necessary control
over suspension and the like.  We already know we want to make changes
in this area, in order to eliminate the use of
jdk.internal.ref.Cleaner (JDK-8149925).  But we've also agreed that
JDK-8149925 wasn't urgent and to defer it to JDK 10.  I don't think we
should revisit that.


Is JDK-8149925 the correct bug id? Looks like that bug has already been fixed 
in 9.


That CR was originally for removing all the uses.  During review the
nio.Bits / direct buffer use was separated, and is the last one.
There was a bunch of discussion about removing that final one in that
review (especially later parts)
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/038663.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039315.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/039862.html

and also here:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039561.html.

However, I don't see a followup CR for removing the nio.Bits use.
Maybe that didn't get created when we split the issue, or maybe I'm
just failing to find it.  Peter?

Note that the consensus back in March/April was to defer it, along
with the dependent actual removal of the internal Cleaner, to JDK 10.

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Kim,

On 2016-08-01 20:47, Kim Barrett wrote:

This updated webrev addresses concerns about the performance of the VM
API used by the reference processor thread in the original webrev.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03/
incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03.incr/
  http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03.incr/


Overall I think you managed to hit a good balance here, keeping the VM 
API straight forward without loosing flexibility. Having the 
ReferenceHandler doing the actual work seems sound and avoids some 
complexity, which I like.


I have one suggestion though, regarding CheckReferencePendingList(). 
While reviewing I found that I had to check several times what its 
return value actually meant, the "check" part of the name doesn't quite 
reveal that. Further, it seems to me that the waiting path of this 
function has fairly little in common with the non-waiting path, e.g. it 
always returns true. So, to make both the naming and implementation more 
clear I'd like to suggest that we split this into two separate 
functions, HasReferencePendingList() and WaitForReferencePendingList(), 
like this:



JVM_ENTRY(jboolean, JVM_HasReferencePendingList(JNIEnv* env))
  JVMWrapper("JVM_HasReferencePendingList");
  MonitorLockerEx ml(Heap_lock);
  return Universe::has_reference_pending_list();
JVM_END

JVM_ENTRY(void, JVM_WaitForReferencePendingList(JNIEnv* env))
  JVMWrapper("JVM_WaitForReferencePendingList");
  MonitorLockerEx ml(Heap_lock);
  while (!Universe::has_reference_pending_list()) {
ml.wait();
  }
JVM_END


And of course, this would then also be reflected in j.l.r.Reference, 
which would have:


private static native boolean hasReferencePendingList();
private static native void waitForReferencePendingList();


Other than this I think the patch looks good.



The originally offered approach was an attempt to minimize changes.  I
was trying to be as similar to the existing code as possible, while
moving part of it into the VM, where we have the necessary control
over suspension and the like.  We already know we want to make changes
in this area, in order to eliminate the use of
jdk.internal.ref.Cleaner (JDK-8149925).  But we've also agreed that
JDK-8149925 wasn't urgent and to defer it to JDK 10.  I don't think we
should revisit that.


Is JDK-8149925 the correct bug id? Looks like that bug has already been 
fixed in 9.


cheers,
Per



As Peter pointed out, my original change introduces a small
performance regression on a microbenchmark for reference processing
throughput.  It also showed a small performance benefit on a different
microbenchmark (allocation rate for a stress test of direct byte
buffers), as noted in the original RFR.  I can reproduce both of
those.

I think reference processing thread throughput is the right measure to
use, so long as others don't become horrible.  Focusing on that, it's
better to just let the reference processing thread do the processing,
rather than slowing it down to allow for the rare case when there's
another thread that could possibly help.  This is especially true now
that Cleaner has such limited usage.

This leads to a different API for other threads; rather than
tryHandlePending that other threads can call to help and to examine
progress, we now have waitForReferenceProcessing.  The VM API is also
different; rather than popReferencePendingList to get or wait, we now
have getAndClearReferencePendingList and checkReferencePendingList,
the latter with an optional wait.

The splitting of the VM API allows us to avoid a narrow race condition
discussed by Peter in his prototypes.  Peter suggested this race was
ok because java.nio.Bits makes several retries.  However, those
retries are only done before throwing OOME.  There are no retries
before invoking GC, so this race could lead to unnecessary successive
GCs.

Doing all the processing on the reference processing thread eliminates
execution of Cleaners on application threads, though that's not nearly
as much an issue now that the use of Cleaner is so restricted.

We've also eliminated a pre-existing issue where a helper could report
no progress even though the reference processing thread (and other
helpers) had removed a pending reference, but not yet processed it.
This could result in the non-progressing helper invoking GC or
reporting failure, even though it might have succeeded if it had
waited for the other thread(s) to complete processing the reference(s)
being worked on.

I think the new waitForReferenceProcessing could be used to fix
JDK-6306116, though that isn't part of this change, and was not really
a motivating factor.

I don't know if the new waitForReferenceProcessing will be used by
whatever we eventually do for JDK-8149925, but I think the new VM API
will suffice for that.  That is, I think JDK-8149925 might require
changes to the core-libs API used 

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Kim,

On 2016-06-24 22:09, Kim Barrett wrote:

Please review this change which moves the Reference pending list and
locking from the java.lang.ref.Reference class into the VM.  This
includes elimination of the ReferencePendingListLocker mechanism in
the VM. This fixes various fragility issues arising from having the
management of this list split between Java and the VM (GC).

This change addresses JDK-8156500 by eliminating the possibility of
suspension while holding the lock. Because the locking is now done in
the VM, we have full control over where suspension may occur.

This change retains the existing interface between the reference
processor and the nio.Bits package, e.g. Reference.tryHandlePending
has the same signature and behavior; this change just pushes the
pending list manipulation down into the VM.  There are open bugs
reports, enhancement requests, and discussions related to that
interface; see below. This change does not attempt to address them.

This change additionally addresses or renders moot
https://bugs.openjdk.java.net/browse/JDK-8055232
(ref) Exceptions while processing Reference pending list

and
https://bugs.openjdk.java.net/browse/JDK-7103238
Ensure pending list lock is held on behalf of GC before enqueuing references on 
to the pending list

It is also relevant for followup discussion of
https://bugs.openjdk.java.net/browse/JDK-8022321
java/lang/ref/OOMEInReferenceHandler.java fails immediately

and
https://bugs.openjdk.java.net/browse/JDK-8149925
We don't need jdk.internal.ref.Cleaner any more - part 1

and has implications for:
https://bugs.openjdk.java.net/browse/JDK-8066859
java/lang/ref/OOMEInReferenceHandler.java failed with java.lang.Exception: 
Reference Handler thread died

In addition to the obviously relevant changes, there are a couple of
changes whose presence here might seem surprising and require further
explanation.

- Removal of a stale comment in create_vm, noticed because it was near
some code being removed as part of this change.  The comment was
rendered obsolete by JDK-8028275.

- Moved initialization of exception classes earlier in
initialize_java_lang_classes. The previous order only worked by
accident, at least for OOME.  During the bulk of the library
initialization, OOME may be thrown, perhaps due to poorly chosen
command line options.  That attempts to use one of the pre-allocated
OOME objects, and tries to fill in its stack trace.  If the Throwable
class is not yet initialized, this leads to a failed assert in the VM
because Throwable.UNASSIGNED_STACK still has a null value.  This
initialization order issue was being masked by the old pending list
implementation; the initialization of Reference ensured
InterruptedException was initialized (thereby initializing its
Throwable base class), and the initialization of Reference just
happened to occur early enough that Throwable was initialized by the
time it was needed when running certain tests.  The forced
initialization of InterruptedException is no longer needed by
Reference, but removal of it triggered test failures (and could
trigger corresponding product failures) because of this ordering
issue.

CR:
https://bugs.openjdk.java.net/browse/JDK-8156500


Patch looks good. The only thing I don't feel qualified to review is the 
initialization order change in thread.cpp, so I'll let others comments 
on that.


I like the pop-one-reference-at-a-time semantics, which simplifies 
things a lot and keeps the interface nice and clean. I was previously 
afraid that it might cause a noticeable performance degradation compared 
to lifting the whole list into Java in one go, but your testing seem to 
prove that's not the case.


cheers,
Per



Webrev:
http://cr.openjdk.java.net/~kbarrett/8156500/jdk.00/
http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.00/

Testing:
RBT ad hoc nightly runtime & gc tests.

With the proposed patch for JDK-8153711 applied, locally ran nearly
35K iterations of the OomDebugTest that is part of that patch, with no
failures / deadlocks.  Without this change it would typically only take
on the order of 100 iterations to hit a deadlock failure.

Locally ran direct byte buffer allocation test:
jdk/test/java/nio/Buffer/DirectBufferAllocTest.java
This change reported 3% faster allocation times, and 1/2 the standard
deviation for allocation times.

Locally ran failing test from JDK-8022321 and JDK-8066859 a few
thousand times.
jdk/test/java/lang/ref/OOMEInReferenceHandler.java
No errors, but failures were noted as hard to reproduce.

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Dan,

On 2016-06-28 03:34, Daniel D. Daugherty wrote:
[...]


src/share/vm/gc/shared/vmGCOperations.cpp
 L103: Heap_lock->unlock();
 You did not add a conditional "Heap_lock->notify_all()" before
 the Heap_lock->unlock() like you've done in other places.
 Since you deleted a release_and_notify_pending_list_lock()
 call, I would think you need the:

   if (Universe::has_reference_pending_list()) {
 Heap_lock->notify_all();
   }


This path is only taken if the GC was skipped and never ran. In this 
case, we know we didn't discover and enqueued any new references on the 
pending list, hence no need to notify any waiting thread (the Reference 
Handler thread in this case).


cheers,
Per

Re: RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi David,

On 2016-06-28 06:04, David Holmes wrote:

Hi Kim,

I like all the removal of the pending-list-locker related code, but
can't really comment on the details of how it was replaced and interacts
with all the GC code. The interface to the Reference class is fine, it
is the GC code that pushes into the reference queue that I can't really
comment on.

I have a couple of queries following up from Coleen's and Dan's reviews.

First, the synchronization of the pending-reference-list leaves me
somewhat perplexed. As Coleen noted you require the heap_lock to be held
but also use an Atomic::xchg. You have a comment:

+   // owns the lock.  Swap is used by parallel non-concurrent reference
+   // processing threads, where some higher level controller owns
+   // Heap_lock, so requires the lock is locked, but not necessarily by
+   // the current thread.

Can you clarify the higher-level protocol that is at play there.
Asserting that "someone" owns a lock seems only marginally useful if you
can't be sure who that owner is, or when they might release it. Some
more direct detecting of being within this higher-level protocol would
be better, if it is possible to detect.


The Heap_lock protects the pending list from access from the Reference 
Handler thread (or any other non-GC thread). During a GC, we grab the 
Heap_lock so that the GC then "owns" the list and is free to modify it. 
However, the GC itself potentially has many GC worker threads doing 
reference enqueuing. The atomic swap is there to allow concurrent 
insertion (prepending) into the list by these GC worker threads. So, 
each GC worker concurrently calls into 
ReferenceProcessor::enqueue_discovered_reflist(), with it's own/local 
list of references to enqueue onto the global pending list.


Hope that clarifies things.

cheers,
Per



---

As previously mentioned the change to the initialization order is a
concern to me - there are always unexpected consequences of making such
changes, no matter how straight-forward they seem at the time.
Pre-initialization of exception classes is not really essential as the
attempt to throw any of them from Java code will force initialization
anyway - it's only for exceptions created and thrown from the VM code
that direct initialization is needed. The problematic case is OOME
because throwing the OOME may trigger a secondary OOME during that class
initialization etc. Hence we try to deal with that by pre-allocating
instances that do not have stacktraces etc, so they can be thrown
safely. Hence my earlier comment that I think the real bug in your
situation is that gen_out_of_memory_error() is not considering how far
into the VM init sequence we are, and that it should be returning the
pre-allocated instance with no backtrace.

---

A query on the JDK side:

src/java.base/share/classes/java/lang/ref/Reference.java

  private static native Reference
popReferencePendingList(boolean wait);

I'm intrigued by the use of the lower-bounded wildcard using Object. As
Object has no super-types this looks very strange and should be
equivalent to the more common upper-bounded wildcard using Object ie
Reference or more concisely Reference. I see this
construct exists in the current code for the ReferenceQueue, but I am
quite intrigued as to why? (Someone getting ready for Any-types? :) )

Thanks,
David
-

On 25/06/2016 6:09 AM, Kim Barrett wrote:

Please review this change which moves the Reference pending list and
locking from the java.lang.ref.Reference class into the VM.  This
includes elimination of the ReferencePendingListLocker mechanism in
the VM. This fixes various fragility issues arising from having the
management of this list split between Java and the VM (GC).

This change addresses JDK-8156500 by eliminating the possibility of
suspension while holding the lock. Because the locking is now done in
the VM, we have full control over where suspension may occur.

This change retains the existing interface between the reference
processor and the nio.Bits package, e.g. Reference.tryHandlePending
has the same signature and behavior; this change just pushes the
pending list manipulation down into the VM.  There are open bugs
reports, enhancement requests, and discussions related to that
interface; see below. This change does not attempt to address them.

This change additionally addresses or renders moot
https://bugs.openjdk.java.net/browse/JDK-8055232
(ref) Exceptions while processing Reference pending list

and
https://bugs.openjdk.java.net/browse/JDK-7103238
Ensure pending list lock is held on behalf of GC before enqueuing
references on to the pending list

It is also relevant for followup discussion of
https://bugs.openjdk.java.net/browse/JDK-8022321
java/lang/ref/OOMEInReferenceHandler.java fails immediately

and
https://bugs.openjdk.java.net/browse/JDK-8149925
We don't need jdk.internal.ref.Cleaner any more - part 1

and has implications for:
https://bugs.openjdk.java.net/browse/JDK-8066859
java/lang/ref/OOMEInRef

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

[Per Liden]

Hi Peter,

On 2016-03-28 19:18, Peter Levart wrote:
[...]

And now a few words about ReferenceHandler thread and synchronization
with it (for Kim and Per mostly). I think it should not be a problem to
move the following two java.lang.ref.Reference methods to native code if
desired:

 static Reference getPendingReferences(int[] discoveryPhaseHolder)
 static int getDiscoveryPhase()

The 1st one is only invoked by a ReferenceHandler thread while the 2nd
is invoked by arbitrary thread. The difference between this and
webrev.09.part2 is that there's no need any more for ReferenceHandler
thread to notify the thread executing the 2nd method and that there's no
need for the 2nd method to perform any waiting. It just needs to obtain
the lock briefly so that it can read the consistent state of two
fields.  Those two fields are Java static fields currently:
Reference.pending & Reference.discoveryPhase and those two methods are
Java methods, but they could be moved to native code if desired to make
the protocol between VM and Java code more robust.

So Kim, Per, what do you think of supporting those 2 methods in native
code? Would that present any problem?


In the best of worlds I'd like the VM to be agnostic to how the pending 
list is processed on the core-libs side. However, after looking at it 
briefly I'm not sure if we can get all they way with only providing a 
getPendingReferences() call.


Anyway, assuming we really need something more than just 
getPendingReferences(), I'm not so keen on exposing a phase counter in 
the API. I think I'd rather have something like this:


/** Get the pending list from the VM, blocking until a list exists.
  * Only used by ReferenceHandler.
  */
Reference getPendingReference();

/** Signal that all references has been enqueued.
  * Only used by ReferenceHandler.
  */
void notifyEnqueuedReferences();

/** If references are pending, wait for a notification from
  * ReferenceHandler that they have been enqueued.
  */
void waitForEnqueuedReferences();


The VM would (roughly) implement this as:


JVM_ENTRY(jobject, JVM_GetPendingReferences(JNIEnv* env))
  // Wait until list becomes non-empty
  {
MonitorLockerEx ml(Heap_lock);
while (!Universe::has_reference_pending_list()) {
  ml.wait();
}

_references_pending++;
  }

  // Detach and return list
  oop list = Universe::swap_reference_pending_list(NULL);
  return JNIHandles::make_local(env, list);
JVM_END


JVM_ENTRY(void, JVM_notifyEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  _references_enqueued = _references_pending;
  ml.notify_all();
JVM_END


JVM_ENTRY(void, JVM_WaitForEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  while (Universe::has_reference_pending_list() ||
 _references_pending != _references_enqueued) {
ml.wait();
  }
JVM_END


And the ReferenceHandler would do something like:


...

// Get pending references from the VM
Reference pending_list = getPendingReferences();

// Enqueue references
while (pending_list != null) {
// Unlink
Reference r = pending_list;
pending_list = r.discovered;
r.discovered = null;

// Enqueue
ReferenceQueue q = r.queue;
if (q != ReferenceQueue.NULL) {
q.enqueue(r);
}
}

notifyEnqueuedReferences();

...


And a helper thread would do something like:

   ...
   System.gc();

   waitForEnqueuedReferences();
   ...


So, this should be fairly similar to what you proposed Peter, but with a 
slightly different API.


But I'm still kind of hoping we can find some way to avoid exposing the 
wait/notify functions, for the sake of keeping the protocol minimal.


cheers,
Per



With webrev.11.part2 I get a 40% improvement in throughput vs.
webrev.10.part2 executing DirectBufferAllocTest in 16 allocating threads
on a 4-core i7 CPU.

Regards, Peter

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi Peter,

On 2016-03-23 15:02, Peter Levart wrote:

Hi Per, Kim,

On 03/22/2016 10:24 AM, Per Liden wrote:

So, I imagine the ReferenceHandler could do something like this:

while (true) {
// getPendingReferences() is a downcall to the VM which
// blocks until the pending list becomes non-empty and
// returns the whole list, transferring it to from VM-land
// to Java-land in a safe and robust way.
Reference pending = getPendingReferences();

// Enqueue the references
while (pending != null) {
Reference r = pending;
pending = r.discovered;
r.discovered = null;
ReferenceQueue q = r.queue;
if (q != ReferenceQueue.NULL) {
q.enqueue(r);
}
}
}


...so I checked what it would be needed if there was such
getPendingReferences() native method. It turns out that a single native
method would not be enough to support the precise direct ByteBuffer
allocation. Here's a refactored webrev that introduces a
getPendingReferences() method which could be turned into a native
equivalent one day. There is another native method needed - int
awaitEnqueuePhaseStart():

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.09.part2/


The need for this additional method arises when one wants to combine
reference discovery with enqueueing of discovered references into one
synchronous operation (discoverAndEnqueueReferences()). A direct
ByteBuffer allocating thread wants to trigger reference discovery
(System.gc()) and wait for discovered references to be enqueued before
continuing with direct memory reservation retries. An alternative to
what I have done in above webrev would be a maintenance of a single
enqueuePhase counter on the Java side with usage roughly as:

discoverAndEnqueueReferences() {
 int phase = Reference.getEnqueuePhase();
 System.gc();
 Reference.awaitEnqueuePhaseGreaterThan(phase);
}

But in that case, System.gc() would have to guarantee that after
discovery of no new references, blocked getPendingReferences() would
still return with an empty list of References (null) just to keep the
DBB allocating thread alive. I have tried to do this variant and
unfortunately it can't be reliably performed with current protocol as
getPendingReferences() can only be programmed to return non-empty
Reference lists without ambiguity. I created a DirectBufferAllocOOMETest
to exercise situations where no new Reference(s) are discovered in a GC
round.

So do what do you think - what would it be easier to support:
a) getPendingReferences() returns empty Reference list (null) after a GC
round that discovers no new pending references
b) getPendingReferences() returns when new Reference(s) are discovered
and there is an additional int awaitEnqueuePhaseStart() as defined in
above webrev.


I've prototyped the VM side. I've ignored the "await" issue for now as I 
first just wanted the basic structure up. I'm running out of time for 
today (and I'll be away the rest of the week) but let's continue the 
discussion next week and figure out the "await" details/alternatives.


Webrevs for jdk9/hs-rt:

http://cr.openjdk.java.net/~pliden/reference_pending_list/webrev.0-jdk
http://cr.openjdk.java.net/~pliden/reference_pending_list/webrev.0-hotspot

It passes jdk/test/java/lang/ref/* and our VM tests for reference 
processing.


cheers,
Per

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi,

On 2016-03-23 08:13, Peter Levart wrote:



On 03/22/2016 10:28 PM, Kim Barrett wrote:

On Mar 22, 2016, at 5:24 AM, Per Liden  wrote:
One thing I like about this approach is that it's only the
ReferenceHandler thread that pops of elements from the pending list
and enqueues them. That simplifies things a lot.

I like that too.  And hopefully we really can get rid of
sun.misc.Cleaner (under whatever name).


 From a GC perspective I would however like to get away from the
shared pending list and the pending list lock entirety and instead
provide a VM downcall to get the pending list. The goal would of
course be to have a more robust way of transferring the pending list
to Java land, instead of today's secret handshake which is easy to
get wrong. Also, not requiring the pending list lock (which is a Java
monitor) to be held during a GC would also simplify things a lot on
the GC side. E.g. the ReferencePendingListLockerThread could be
removed completely.

I’ve been thinking along the same lines.  I think having the pending
list (and associated locking and notification) in Java is just making
life difficult for ourselves, and that things could be much simpler if
that whole protocol was owned by the VM.

Once the reference handler thread has obtained the latest list, if it
then wants to publish that list for other Java threads to help
process, that’s a policy choice that can be explored on the Java side,
with no impact on the VM (including the GC).



If the only blocking/waiting of ReferenceHandler thread was performed by
native code, could it simply ignore Java thread interrupts? If this is
possible, then the problems of InterruptedException allocation and
consequent OutOfMemoryError(s) just disappear.


Yes, blocking in the VM here would ignore thread interrupts and not 
throw InterruptedException.


cheers,
Per

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi Peter,

On 2016-03-21 16:30, Peter Levart wrote:

Hi Per,

May I point you to my proposed change in Reference(Handler) for JDK 9,
being discussed in the thread about JDK-8149925. It will hopefully
remove the special-casing of sun.misc.Cleaner, change the way how
pending references are being enqueued by ReferenceHandler thread and how
other thread(s) can synchronize with it. Since you seem to have a great
knowledge of VM part of things, I would very much like to hear what you
think of that change. Here's the latest webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.08.part2/


(see Reference.java and Bits.java for an example of how this
synchronization with ReferenceHandler thread is to be used)


One thing I like about this approach is that it's only the 
ReferenceHandler thread that pops of elements from the pending list and 
enqueues them. That simplifies things a lot.


From a GC perspective I would however like to get away from the shared 
pending list and the pending list lock entirety and instead provide a VM 
downcall to get the pending list. The goal would of course be to have a 
more robust way of transferring the pending list to Java land, instead 
of today's secret handshake which is easy to get wrong. Also, not 
requiring the pending list lock (which is a Java monitor) to be held 
during a GC would also simplify things a lot on the GC side. E.g. the 
ReferencePendingListLockerThread could be removed completely.


So, I imagine the ReferenceHandler could do something like this:

while (true) {
// getPendingReferences() is a downcall to the VM which
// blocks until the pending list becomes non-empty and
// returns the whole list, transferring it to from VM-land
// to Java-land in a safe and robust way.
Reference pending = getPendingReferences();

// Enqueue the references
while (pending != null) {
Reference r = pending;
pending = r.discovered;
r.discovered = null;
ReferenceQueue q = r.queue;
if (q != ReferenceQueue.NULL) {
q.enqueue(r);
}
}
}

I haven't thought through the details when it comes having additional 
Java threads helping out with Cleaners. The ReferenceHandler would be 
free to use whatever lists/locks is wants to handle this and the GC 
wouldn't know anything about it. But, with the above approach at least 
the interface between the ReferenceHandler and the VM would be pretty 
clear and hard(er) to misuse.


cheers,
Per



Regards, Peter

On 03/21/2016 04:13 PM, Peter Levart wrote:

Hi Per, David,

As things stand, there is a very good chance that sun.misc.Cleaner
will go away in JDK9, so all this speculation about the source of
OOME(s) can be put to rest. But for JDK 8u, I agree that this should
be sorted out.

My feeling is that (instanceof Cleaner) can not result in allocation
and therefore can not trigger OOME if the Cleaner class is already
loaded at that time. I think that we were chasing the wrong rabbit. As
I have found later, there is a much more probable cause for
ReferenceHandler thread dying with OOME after the fix to catch OOME
from lock.wait(). It is triggered by the invocation of Cleaner.clean()
later down in the code. I even created a reproducer for it. See my
last two comments of the following issue:

https://bugs.openjdk.java.net/browse/JDK-8066859

(but don't look at the proposed fix since it is not very good)


I think that for JDK 8u we could revert the code and do (instanceof
Cleaner) checks outside the synchronized block and in addition, find a
way to handle the OOME thrown from Cleaner.clean().

What do you think?


Regards, Peter


On 03/21/2016 02:41 PM, Per Liden wrote:

Hi David,

On 2016-03-21 13:49, David Holmes wrote:

Hi Per,

On 21/03/2016 10:20 PM, Per Liden wrote:

Hi Peter & David,

(Resurrecting an old thread here...)

On 2014-01-22 03:19, David Holmes wrote:

Hi Peter,

On 22/01/2014 12:00 AM, Peter Levart wrote:

Hi, David, Kalyan,

Summing up the discussion, I propose the following patch for
ReferenceHandler:

http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/






I can live with it, though it maybe that once Cleaner has been
preloaded
instanceof can no longer throw OOME. Can't be 100% sure. And there's
some duplication/verbosity in the commentary that could be trimmed
down :)


While investigating a Reference pending list issue on the GC side of
things I looked at the ReferenceHandler thread and noticed something
which made me uneasy. The fix for JDK-8022321 added pre-loading of the
Cleaer class to avoid OMME, but also moved the "instanceof Cleaner"
inside the try/catch with a comment that it "sometimes" can throw an
OOME. I understand this was done because we're not 100% sure if a OOME
can still happen here, despite the pre-loading.

However, if it can throw an OOME that means it's allocating, which in
turn mea

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

On 2016-03-21 18:32, Kim Barrett wrote:

On Mar 21, 2016, at 8:20 AM, Per Liden  wrote:

Hi Peter & David,

(Resurrecting an old thread here...)

On 2014-01-22 03:19, David Holmes wrote:

Hi Peter,

On 22/01/2014 12:00 AM, Peter Levart wrote:

Hi, David, Kalyan,

Summing up the discussion, I propose the following patch for
ReferenceHandler:

http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/



I can live with it, though it maybe that once Cleaner has been preloaded
instanceof can no longer throw OOME. Can't be 100% sure. And there's
some duplication/verbosity in the commentary that could be trimmed down :)


While investigating a Reference pending list issue on the GC side of things I looked at the 
ReferenceHandler thread and noticed something which made me uneasy. The fix for JDK-8022321 added 
pre-loading of the Cleaer class to avoid OMME, but also moved the "instanceof Cleaner" 
inside the try/catch with a comment that it "sometimes" can throw an OOME. I understand 
this was done because we're not 100% sure if a OOME can still happen here, despite the pre-loading.

However, if it can throw an OOME that means it's allocating, which in turn means it can provoke a 
GC. If that happens, it looks to me like we have a bug here. The ReferenceHandler thread is not 
allowed to provoke a GC while it's holding on to the pending list lock, since the pending list 
might be updated during a GC and "pending = r.discovered" will than overwrite something 
other than "r", silently dropping any newly discovered References which will never be 
discovered by the the GC again.

On the other hand, if an OOME can never happen (i.e. no GC) here then we're 
good the comment is just incorrect. The instanceof check could be moved out of 
the try/catch block again, like it was prior to this change, just to make it 
obvious that we will not be able to cause new allocations inside the critical 
section. Or at a minimum, the comment saying OOME can still happen should be 
adjusted.

Thoughts?

thanks,
Per

Btw, to the best of my knowledge, the pre-loading of Cleaner should avoid any 
GC activity from instanceof, but I can't say that am a 100% sure either.


Per - I think you are raising the same issue as discussed in 
https://bugs.openjdk.java.net/browse/JDK-8055232.


Ah, thanks Kim for pointing that out.

cheers,
Per

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi Peter,

On 2016-03-21 16:13, Peter Levart wrote:

Hi Per, David,

As things stand, there is a very good chance that sun.misc.Cleaner will
go away in JDK9, so all this speculation about the source of OOME(s) can
be put to rest. But for JDK 8u, I agree that this should be sorted out.

My feeling is that (instanceof Cleaner) can not result in allocation and
therefore can not trigger OOME if the Cleaner class is already loaded at
that time. I think that we were chasing the wrong rabbit. As I have
found later, there is a much more probable cause for ReferenceHandler
thread dying with OOME after the fix to catch OOME from lock.wait(). It
is triggered by the invocation of Cleaner.clean() later down in the
code. I even created a reproducer for it. See my last two comments of
the following issue:

https://bugs.openjdk.java.net/browse/JDK-8066859

(but don't look at the proposed fix since it is not very good)


I think that for JDK 8u we could revert the code and do (instanceof
Cleaner) checks outside the synchronized block and in addition, find a
way to handle the OOME thrown from Cleaner.clean().

What do you think?


That sound good to me. With the addition of the try/catch around 
Cleaner.clean() catching not just OOME, but all Throwables, right?


cheers,
Per




Regards, Peter


On 03/21/2016 02:41 PM, Per Liden wrote:

Hi David,

On 2016-03-21 13:49, David Holmes wrote:

Hi Per,

On 21/03/2016 10:20 PM, Per Liden wrote:

Hi Peter & David,

(Resurrecting an old thread here...)

On 2014-01-22 03:19, David Holmes wrote:

Hi Peter,

On 22/01/2014 12:00 AM, Peter Levart wrote:

Hi, David, Kalyan,

Summing up the discussion, I propose the following patch for
ReferenceHandler:

http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/






I can live with it, though it maybe that once Cleaner has been
preloaded
instanceof can no longer throw OOME. Can't be 100% sure. And there's
some duplication/verbosity in the commentary that could be trimmed
down :)


While investigating a Reference pending list issue on the GC side of
things I looked at the ReferenceHandler thread and noticed something
which made me uneasy. The fix for JDK-8022321 added pre-loading of the
Cleaer class to avoid OMME, but also moved the "instanceof Cleaner"
inside the try/catch with a comment that it "sometimes" can throw an
OOME. I understand this was done because we're not 100% sure if a OOME
can still happen here, despite the pre-loading.

However, if it can throw an OOME that means it's allocating, which in
turn means it can provoke a GC. If that happens, it looks to me like we
have a bug here. The ReferenceHandler thread is not allowed to
provoke a
GC while it's holding on to the pending list lock, since the pending
list might be updated during a GC and "pending = r.discovered" will
than
overwrite something other than "r", silently dropping any newly
discovered References which will never be discovered by the the GC
again.


Then the code was completely broken because it was obviously capable of
allocating whilst holding the lock. There is nothing in the Java code to
indicate allocation should not happen and no way that Java code can
directly control that! We were only fixing the problem of the exception
killing the thread, not trying to address an undisclosed illegal
allocation problem!


JDK-8022321 did indeed fix a real issue. It might also have
unintentionally introduced a new one. Prior to JDK-8022321 we knew
that the ReferenceHandler couldn't provoke a GC while manipulating the
pending list, since the code was:

synchronized (lock) {
if (pending != null) {
r = pending;
pending = r.discovered;
r.discovered = null;
} else {

}
}

The manipulation of the pending list is built on some secret/ugly
rules and handshakes between the GC and the ReferenceHandler, which
only works because we control of both.



How would a GC thread update pending if the ReferenceHandlerThread holds
the lock?


The pending list lock is grabbed by the Java thread issuing the VM
operation, on behalf of the GC to allow the GC the manipulate the
pending list. If the thread issuing the VM operation is the
ReferenceHandler, then the monitor is taken recursively, which is ok
as long as ReferenceHandler isn't in the middle of unlinking an element.




On the other hand, if an OOME can never happen (i.e. no GC) here then
we're good the comment is just incorrect. The instanceof check could be
moved out of the try/catch block again, like it was prior to this
change, just to make it obvious that we will not be able to cause new
allocations inside the critical section. Or at a minimum, the comment
saying OOME can still happen should be adjusted.


I found it very difficult to determine with 100% certainty whether or
not the instanceof could ever trigger an allocation and hence
potentially an OOME.


I agree, it&

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi David,

On 2016-03-21 13:49, David Holmes wrote:

Hi Per,

On 21/03/2016 10:20 PM, Per Liden wrote:

Hi Peter & David,

(Resurrecting an old thread here...)

On 2014-01-22 03:19, David Holmes wrote:

Hi Peter,

On 22/01/2014 12:00 AM, Peter Levart wrote:

Hi, David, Kalyan,

Summing up the discussion, I propose the following patch for
ReferenceHandler:

http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/





I can live with it, though it maybe that once Cleaner has been preloaded
instanceof can no longer throw OOME. Can't be 100% sure. And there's
some duplication/verbosity in the commentary that could be trimmed
down :)


While investigating a Reference pending list issue on the GC side of
things I looked at the ReferenceHandler thread and noticed something
which made me uneasy. The fix for JDK-8022321 added pre-loading of the
Cleaer class to avoid OMME, but also moved the "instanceof Cleaner"
inside the try/catch with a comment that it "sometimes" can throw an
OOME. I understand this was done because we're not 100% sure if a OOME
can still happen here, despite the pre-loading.

However, if it can throw an OOME that means it's allocating, which in
turn means it can provoke a GC. If that happens, it looks to me like we
have a bug here. The ReferenceHandler thread is not allowed to provoke a
GC while it's holding on to the pending list lock, since the pending
list might be updated during a GC and "pending = r.discovered" will than
overwrite something other than "r", silently dropping any newly
discovered References which will never be discovered by the the GC again.


Then the code was completely broken because it was obviously capable of
allocating whilst holding the lock. There is nothing in the Java code to
indicate allocation should not happen and no way that Java code can
directly control that! We were only fixing the problem of the exception
killing the thread, not trying to address an undisclosed illegal
allocation problem!


JDK-8022321 did indeed fix a real issue. It might also have 
unintentionally introduced a new one. Prior to JDK-8022321 we knew that 
the ReferenceHandler couldn't provoke a GC while manipulating the 
pending list, since the code was:


synchronized (lock) {
if (pending != null) {
r = pending;
pending = r.discovered;
r.discovered = null;
} else {

}
}

The manipulation of the pending list is built on some secret/ugly rules 
and handshakes between the GC and the ReferenceHandler, which only works 
because we control of both.




How would a GC thread update pending if the ReferenceHandlerThread holds
the lock?


The pending list lock is grabbed by the Java thread issuing the VM 
operation, on behalf of the GC to allow the GC the manipulate the 
pending list. If the thread issuing the VM operation is the 
ReferenceHandler, then the monitor is taken recursively, which is ok as 
long as ReferenceHandler isn't in the middle of unlinking an element.





On the other hand, if an OOME can never happen (i.e. no GC) here then
we're good the comment is just incorrect. The instanceof check could be
moved out of the try/catch block again, like it was prior to this
change, just to make it obvious that we will not be able to cause new
allocations inside the critical section. Or at a minimum, the comment
saying OOME can still happen should be adjusted.


I found it very difficult to determine with 100% certainty whether or
not the instanceof could ever trigger an allocation and hence
potentially an OOME.


I agree, it's not obvious.

cheers,
Per



With JVMCI it is now easier to imagine that compilation of this code by
a JVMCI compiler might lead to allocation while the lock is held!

Cheers,
David


Thoughts?

thanks,
Per

Btw, to the best of my knowledge, the pre-loading of Cleaner should
avoid any GC activity from instanceof, but I can't say that am a 100%
sure either.



Any specific reason to use Unsafe to do the preload rather than
Class.forName ? Does this force Unsafe to be loaded earlier than it
otherwise would?

Thanks,
David



all 10 java/lang/ref tests pass on my PC (including
OOMEInReferenceHandler).

I kindly ask Kalyan to try to re-run the OOMEInReferenceHandler test
with this code and report any failure.


Thanks, Peter

On 01/21/2014 08:57 AM, David Holmes wrote:

On 21/01/2014 4:54 PM, Peter Levart wrote:


On 01/21/2014 03:22 AM, David Holmes wrote:

Hi Peter,

I do not see Cleaner being loaded prior to the main class on either
Windows or Linux. Which platform are you on? Did you see it loaded
before the main class or as part of executing it?


Before. The main class is empty:

public class Test { public static void main(String... a) {} }

Here's last few lines of -verbose:class:

[Loaded java.util.TimeZone from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneI

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

[Per Liden]

Hi Peter & David,

(Resurrecting an old thread here...)

On 2014-01-22 03:19, David Holmes wrote:

Hi Peter,

On 22/01/2014 12:00 AM, Peter Levart wrote:

Hi, David, Kalyan,

Summing up the discussion, I propose the following patch for
ReferenceHandler:

http://cr.openjdk.java.net/~plevart/jdk9-dev/OOMEInReferenceHandler/webrev.01/



I can live with it, though it maybe that once Cleaner has been preloaded
instanceof can no longer throw OOME. Can't be 100% sure. And there's
some duplication/verbosity in the commentary that could be trimmed down :)


While investigating a Reference pending list issue on the GC side of 
things I looked at the ReferenceHandler thread and noticed something 
which made me uneasy. The fix for JDK-8022321 added pre-loading of the 
Cleaer class to avoid OMME, but also moved the "instanceof Cleaner" 
inside the try/catch with a comment that it "sometimes" can throw an 
OOME. I understand this was done because we're not 100% sure if a OOME 
can still happen here, despite the pre-loading.


However, if it can throw an OOME that means it's allocating, which in 
turn means it can provoke a GC. If that happens, it looks to me like we 
have a bug here. The ReferenceHandler thread is not allowed to provoke a 
GC while it's holding on to the pending list lock, since the pending 
list might be updated during a GC and "pending = r.discovered" will than 
overwrite something other than "r", silently dropping any newly 
discovered References which will never be discovered by the the GC again.


On the other hand, if an OOME can never happen (i.e. no GC) here then 
we're good the comment is just incorrect. The instanceof check could be 
moved out of the try/catch block again, like it was prior to this 
change, just to make it obvious that we will not be able to cause new 
allocations inside the critical section. Or at a minimum, the comment 
saying OOME can still happen should be adjusted.


Thoughts?

thanks,
Per

Btw, to the best of my knowledge, the pre-loading of Cleaner should 
avoid any GC activity from instanceof, but I can't say that am a 100% 
sure either.




Any specific reason to use Unsafe to do the preload rather than
Class.forName ? Does this force Unsafe to be loaded earlier than it
otherwise would?

Thanks,
David



all 10 java/lang/ref tests pass on my PC (including
OOMEInReferenceHandler).

I kindly ask Kalyan to try to re-run the OOMEInReferenceHandler test
with this code and report any failure.


Thanks, Peter

On 01/21/2014 08:57 AM, David Holmes wrote:

On 21/01/2014 4:54 PM, Peter Levart wrote:


On 01/21/2014 03:22 AM, David Holmes wrote:

Hi Peter,

I do not see Cleaner being loaded prior to the main class on either
Windows or Linux. Which platform are you on? Did you see it loaded
before the main class or as part of executing it?


Before. The main class is empty:

public class Test { public static void main(String... a) {} }

Here's last few lines of -verbose:class:

[Loaded java.util.TimeZone from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfo from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded sun.util.calendar.ZoneInfoFile$1 from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInput from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
[Loaded java.io.DataInputStream from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]
*[Loaded sun.misc.Cleaner from
/home/peter/Apps64/jdk1.8.0-ea-b121/jre/lib/rt.jar]*


Curious. I wonder what the controlling factor is ??



I'm on linux, 64bit and using official EA build 121 of JDK 8...

But if I try with JDK 7u45, I don't see it. So perhaps it would be good
to trigger Cleaner loading and initialization as part of
ReferenceHandler initialization to play things safe.



If we do that for Cleaner we may as well do it for
InterruptedException too.


Also, it is not that I think ReferenceHandler is responsible for
reporting OOME, but that it is responsible for reporting that it was
unable to perform a clean or enqueue because of OOME.


This would be necessary if we skipped a Reference because of OOME, but
if we just re-try until we eventually succeed, nothing is lost, nothing
to report (but a slow response)...


Agreed - just trying to clarify things.



Your suggested approach seems okay though I'm not sure why we
shouldn't help things along by calling System.gc() ourselves rather
than just yielding and hoping things will get cleaned up elsewhere.
But for the present purposes your approach will suffice I think.


Maybe my understanding is wrong but isn't the fact that OOME is rised a
consequence of that VM has already attempted to clear things up
(executing a GC round synchronously) but didn't succeed to make enough
free space to satisfy the allocation request? If this is only how some
collectors/allocators are implemented and not a general rule, then we
s

Re: RFR: 8071507: (ref) Clear phantom reference as soft and weak references do

[Per Liden]

On 2015-12-04 19:16, Kim Barrett wrote:

On Dec 4, 2015, at 2:16 AM, Per Liden  wrote:


On 2015-12-02 19:37, Kim Barrett wrote:

Please review this change to PhantomReference processing, changing the
GC-based notification to automatically clear the referent.

[…]
CR:
https://bugs.openjdk.java.net/browse/JDK-8071507

Webrevs:
http://cr.openjdk.java.net/~kbarrett/8071507/jdk.05/


test/java/lang/ref/PhantomReferentClearing.java:

  85 // Delete root -> O1, collect, verify P1 notified, P2 not notified.
  86 O1 = null;
  87 System.gc();
  88 if (Q1.remove(ENQUEUE_TIMEOUT) == null) {
  89 throw new RuntimeException("P1 not notified by O1 deletion");
  90 } else if (Q2.remove(ENQUEUE_TIMEOUT) != null) {
  91 throw new RuntimeException("P2 notified by O1 deletion.");
  92 }
  93
  94 // Delete root -> O2, collect. P2 should be notified.
  95 O2 = null;
  96 System.gc();
  97 if (Q2.remove(ENQUEUE_TIMEOUT) == null) {
  98 throw new RuntimeException("P2 not notified by O2 deletion");
  99 }

The calls to System.gc() isn't guaranteed to do what the test expects here. As 
you know, System.gc(), might not actually do anything, depending on which 
collector is used and the current circumstances (GC locker held, a concurrent 
GC is already in process, etc). To make the test robust I'd suggest you call 
System.gc() and check the queue in a loop, and fail after some reasonable 
amount of time/iterations.


I'd rather not clutter and uglify this and other tests to deal with a
problem we don't (so far as I can tell) presently actually have.  If a
problem does arise, the form of that problem will help guide what the
solution needs to be.  I suspect adding something to WhiteBox would be
the proper approach, so that it can be shared with other tests of this
form.


We do actually have this problem today, i.e. there are cases where our 
collectors will not do a "full GC" as a result of a call to System.gc(). 
One example would be if the GC-locker is active. However, it's seems 
very unlikely that this particular test would provoking such a condition.


Our current WhiteBox API for FullGC is prone to the same issue. Maybe it 
would be a good idea to strengthen that API and used that in the future 
for tests like this.


I understand if you don't want to take on this problem here and now, 
given that we have other test which is very similar to this one, and 
they seem to have worked so far without issues.


cheers,
/Per

Re: RFR: 8071507: (ref) Clear phantom reference as soft and weak references do

[Per Liden]

Hi Peter,

On 2015-12-04 13:35, Peter Levart wrote:



On 12/04/2015 08:16 AM, Per Liden wrote:


test/java/lang/ref/PhantomReferentClearing.java:

  85 // Delete root -> O1, collect, verify P1 notified, P2 not
notified.
  86 O1 = null;
  87 System.gc();
  88 if (Q1.remove(ENQUEUE_TIMEOUT) == null) {
  89 throw new RuntimeException("P1 not notified by O1
deletion");
  90 } else if (Q2.remove(ENQUEUE_TIMEOUT) != null) {
  91 throw new RuntimeException("P2 notified by O1
deletion.");
  92 }
  93
  94 // Delete root -> O2, collect. P2 should be notified.
  95 O2 = null;
  96 System.gc();
  97 if (Q2.remove(ENQUEUE_TIMEOUT) == null) {
  98 throw new RuntimeException("P2 not notified by O2
deletion");
  99 }

The calls to System.gc() isn't guaranteed to do what the test expects
here. As you know, System.gc(), might not actually do anything,
depending on which collector is used and the current circumstances (GC
locker held, a concurrent GC is already in process, etc). To make the
test robust I'd suggest you call System.gc() and check the queue in a
loop, and fail after some reasonable amount of time/iterations.


Whether you poll the queue for some time T or you remove from queue with
timeout of T, it doesn't matter. The Reference(s) are enqueued by a
ReferenceHandler thread and the thread waiting in remove() will get
notified when a reference gets enqueued...


I think you maybe missed my point. There's no guarantee that a call to 
System.gc() will cause reference processing to happen at all, and if it 
happens there's no guarantee that it will discover P1/P2 here. In this 
specific test it's very likely to happen, but that's a different story. 
I still don't think this test should rely on a behavior that isn't 
guaranteed by the spec.


cheers,
/Per



Regards, Peter



cheers,
/Per


http://cr.openjdk.java.net/~kbarrett/8071507/hotspot.05/

Testing:
jprt, aurora ad hoc (defaults, GC/Runtime nightly, JCK)

Re: RFR: 8071507: (ref) Clear phantom reference as soft and weak references do

[Per Liden]

Hi Kim,

On 2015-12-02 19:37, Kim Barrett wrote:

Please review this change to PhantomReference processing, changing the
GC-based notification to automatically clear the referent.

This change provides performance benefits by eliminating the work
involved in keeping the otherwise inaccessible referent objects alive,
as required by the existing specification. This not only immediately
removes some work, but may enable further performance improvements.
It also allows the referent objects to be immediately reclaimed in
the GC cycle in which they were determined to be inaccessible, rather
than lingering as a form of floating garbage until the application
deals with the notified reference.

This change results in a behavioral change to application code, as
demonstrated by the associated test.  Under the old specification, a
reference R with referent X may be kept alive because it is referenced
by an otherwise inaccessible referent Y of phantom reference P.  This
will result in X being treated as strongly referenced and prevent R
from being notified, even if R is a phantom reference and X has become
inaccessible to the application.  With this change, Y is reclaimed
when it becomes inaccessible and P is notified, and no longer prevents
X from itself becoming a candidate for reclaimation once it is no
longer accessible to the application.  While this is a change in
behavior, it seems unlikely to affect applications negatively.

CR:
https://bugs.openjdk.java.net/browse/JDK-8071507

Webrevs:
http://cr.openjdk.java.net/~kbarrett/8071507/jdk.05/


test/java/lang/ref/PhantomReferentClearing.java:

  85 // Delete root -> O1, collect, verify P1 notified, P2 not 
notified.

  86 O1 = null;
  87 System.gc();
  88 if (Q1.remove(ENQUEUE_TIMEOUT) == null) {
  89 throw new RuntimeException("P1 not notified by O1 
deletion");

  90 } else if (Q2.remove(ENQUEUE_TIMEOUT) != null) {
  91 throw new RuntimeException("P2 notified by O1 deletion.");
  92 }
  93
  94 // Delete root -> O2, collect. P2 should be notified.
  95 O2 = null;
  96 System.gc();
  97 if (Q2.remove(ENQUEUE_TIMEOUT) == null) {
  98 throw new RuntimeException("P2 not notified by O2 
deletion");

  99 }

The calls to System.gc() isn't guaranteed to do what the test expects 
here. As you know, System.gc(), might not actually do anything, 
depending on which collector is used and the current circumstances (GC 
locker held, a concurrent GC is already in process, etc). To make the 
test robust I'd suggest you call System.gc() and check the queue in a 
loop, and fail after some reasonable amount of time/iterations.


cheers,
/Per


http://cr.openjdk.java.net/~kbarrett/8071507/hotspot.05/

Testing:
jprt, aurora ad hoc (defaults, GC/Runtime nightly, JCK)