[PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup
Peter Levart
peter.levart at gmail.com
Tue Feb 6 09:15:58 UTC 2018
Hi,
I checked the behavior of Objects.requireNonNull(this) at appropriate
place instead of Reference.reachabilityFence(this) and it does seem to
work. For example in the following test (see method m()):
import java.util.Objects;
import java.util.concurrent.atomic.AtomicLong;
public class ReachabilityTest {
private static final AtomicLong seq = new AtomicLong();
private static final AtomicLong deallocatedHwm = new AtomicLong();
private static long allocate() {
return seq.incrementAndGet();
}
private static void deallocate(long address) {
deallocatedHwm.accumulateAndGet(address, Math::max);
}
private static void access(long address) {
if (deallocatedHwm.get() == address) {
throw new IllegalStateException(
"Address " + address + " has allready been deallocated");
}
}
private long address = allocate();
@SuppressWarnings("deprecation")
@Override
protected void finalize() throws Throwable {
deallocate(address);
address = 0;
}
void m() {
long a = address;
if (a != 0) {
System.gc();
try { Thread.sleep(1); } catch (InterruptedException e) {}
access(a);
// Reference.reachabilityFence(this);
Objects.requireNonNull(this);
}
}
static void test() {
new ReachabilityTest().m();
}
public static void main(String[] args) {
while (true) {
test();
}
}
}
...Objects.requireNonNull does prevent otherwise reliable provoked
failure, just like Reference.reachabilityFence.
As to the speed of optimized-away Objects.requireNonNull() vs.
Reference.reachabilityFence() I tried the following benchmark:
package jdk.test;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import java.lang.ref.Reference;
import java.util.Arrays;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
@BenchmarkMode(Mode.AverageTime)
@Warmup(iterations = 5)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Fork(1)
@State(Scope.Thread)
public class ReachabilityBench {
static class Buf0 {
final byte[] buf;
Buf0(int len) {
buf = new byte[len];
}
@SuppressWarnings("deprecation")
@Override
protected void finalize() throws Throwable {
Arrays.fill(buf, (byte) -1);
}
byte get(int i) {
byte[] b = buf;
// this might get finalized before accessing the b element
byte r = b[i];
return r;
}
}
static class Buf1 {
final byte[] buf;
Buf1(int len) {
buf = new byte[len];
}
@SuppressWarnings("deprecation")
@Override
protected void finalize() throws Throwable {
Arrays.fill(buf, (byte) -1);
}
byte get(int i) {
byte[] b = buf;
byte r = b[i];
Reference.reachabilityFence(this);
return r;
}
}
static class Buf2 {
final byte[] buf;
Buf2(int len) {
buf = new byte[len];
}
@SuppressWarnings("deprecation")
@Override
protected void finalize() throws Throwable {
Arrays.fill(buf, (byte) -1);
}
byte get(int i) {
byte[] b = buf;
byte r = b[i];
Objects.requireNonNull(this);
return r;
}
}
Buf0 buf0;
Buf1 buf1;
Buf2 buf2;
@Param({"64"})
public int len;
@Setup(Level.Trial)
public void setup() {
buf0 = new Buf0(len);
buf1 = new Buf1(len);
buf2 = new Buf2(len);
}
@Benchmark
public int sumBuf0() {
int s = 0;
for (int i = 0; i < len; i++) {
s += buf0.get(i);
}
return s;
}
@Benchmark
public int sumBuf1() {
int s = 0;
for (int i = 0; i < len; i++) {
s += buf1.get(i);
}
return s;
}
@Benchmark
public int sumBuf2() {
int s = 0;
for (int i = 0; i < len; i++) {
s += buf2.get(i);
}
return s;
}
}
The results are as follows:
Benchmark (len) Mode Cnt Score Error Units
ReachabilityBench.sumBuf0 64 avgt 10 20.530 ± 0.052 ns/op
ReachabilityBench.sumBuf1 64 avgt 10 184.402 ± 0.531 ns/op
ReachabilityBench.sumBuf2 64 avgt 10 20.502 ± 0.027 ns/op
Objects.requireNonNull() shows zero overhead here.
I guess the main question is whether Objects.requireNonNull(this)
behavior in the former test is a result of chance and current Hotspot
behavior or is it somehow guaranteed by the spec.
Regards, Peter
On 02/03/2018 02:04 AM, Peter Levart wrote:
> Hi,
>
> I have one question, maybe stupid. I'm wondering about one situation.
> Suppose you have this Java code:
>
> void m() {
> // code before...
>
> Objects.requireNonNull(this);
> }
>
> Of course, the non-null check will never throw NPE. The check will
> most likely even be optimized away by JIT. But is JIT allowed to
> optimize it away so that the "code before" observes the effects of
> 'this' being unreachable? Wouldn't that violate the semantics of the
> program as it would 1st observe that some object is not reachable and
> after that it would observer that 'this' still points to some object
> which by definition must be it?
>
> If JIT is not allowed to optimize the null check so that the object
> becomes unreachable, then Objects.requireNonNull could be used as a
> replacement for Reference.reachabilityFence. It might even perform
> better.
>
> What do you think?
>
> Regards, Peter
>
>
> On 02/01/18 23:50, Paul Sandoz wrote:
>> Hi Ben,
>>
>> I don’t think you require the fence in all the cases you have
>> currently placed it e.g. here for example
>>
>> $memtype$ y = $toBits$(x);
>> UNSAFE.put$Memtype$Unaligned(null, a, y, bigEndian);
>> + Reference.reachabilityFence(this);
>> return this;
>>
>> since “this” is being returned it will be kept live during the unsafe
>> access.
>>
>> Would you mind providing a JMH benchmark and results for the
>> performance with and without the fence for say a put and/or a get. I
>> would like to understand the performance impact on HotSpot, this is
>> one reason why we have yet to add such fences as it will likely
>> impact performance.
>>
>> At the moment we are relying on the method not being inlined, which
>> is an expedient technique to make it functional and keep a reference
>> alive but not necessarily optimal for usages in DBB.
>>
>> For more details see:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8169605
>> <https://bugs.openjdk.java.net/browse/JDK-8169605>
>> https://bugs.openjdk.java.net/browse/JDK-8149610
>> <https://bugs.openjdk.java.net/browse/JDK-8149610>
>>
>> Thanks,
>> Paul.
>>
>>> On Feb 1, 2018, at 6:55 AM, Ben Walsh <ben_walsh at uk.ibm.com> wrote:
>>>
>>> This contribution forms a partial solution to the problem detailed
>>> here -
>>> http://thevirtualmachinist.blogspot.ca/2011/07/subtle-issue-of-reachability.html
>>>
>>> .
>>>
>>> In this context, this contribution provides "markers" such that a
>>> suitably
>>> "aware" compiler can reduce the chances of such a problem occurring
>>> with
>>> each of the Direct<Type>Buffer<RW><BO> objects and MappedByteBuffer
>>> object. The reachabilityFences may prevent crashes / exceptions due to
>>> cleaning up the backing memory before the user has finished using the
>>> pointer.
>>>
>>> Any compiler not suitably "aware" could be modified to make use of the
>>> "markers".
>>>
>>>
>>> I would like to pair with a sponsor to contribute this patch ...
>>>
>>> -------------------------------------------------------------------------------------------------------------------
>>>
>>> diff -r d51e64840b4f
>>> src/java.base/share/classes/java/nio/Direct-X-Buffer-bin.java.template
>>> ---
>>> a/src/java.base/share/classes/java/nio/Direct-X-Buffer-bin.java.template
>>>
>>> Wed Jan 31 12:04:53 2018 +0800
>>> +++
>>> b/src/java.base/share/classes/java/nio/Direct-X-Buffer-bin.java.template
>>>
>>> Thu Feb 01 11:30:10 2018 +0000
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or
>>> modify it
>>> @@ -33,15 +33,21 @@
>>>
>>> private $type$ get$Type$(long a) {
>>> $memtype$ x = UNSAFE.get$Memtype$Unaligned(null, a,
>>> bigEndian);
>>> - return $fromBits$(x);
>>> + $type$ y = $fromBits$(x);
>>> + Reference.reachabilityFence(this);
>>> + return y;
>>> }
>>>
>>> public $type$ get$Type$() {
>>> - return get$Type$(ix(nextGetIndex($BYTES_PER_VALUE$)));
>>> + $type$ y = get$Type$(ix(nextGetIndex($BYTES_PER_VALUE$)));
>>> + Reference.reachabilityFence(this);
>>> + return y;
>>> }
>>>
>>> public $type$ get$Type$(int i) {
>>> - return get$Type$(ix(checkIndex(i, $BYTES_PER_VALUE$)));
>>> + $type$ y = get$Type$(ix(checkIndex(i, $BYTES_PER_VALUE$)));
>>> + Reference.reachabilityFence(this);
>>> + return y;
>>> }
>>>
>>> #end[rw]
>>> @@ -50,6 +56,7 @@
>>> #if[rw]
>>> $memtype$ y = $toBits$(x);
>>> UNSAFE.put$Memtype$Unaligned(null, a, y, bigEndian);
>>> + Reference.reachabilityFence(this);
>>> return this;
>>> #else[rw]
>>> throw new ReadOnlyBufferException();
>>> diff -r d51e64840b4f
>>> src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>>> ---
>>> a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>>> Wed Jan 31 12:04:53 2018 +0800
>>> +++
>>> b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
>>> Thu Feb 01 11:30:10 2018 +0000
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2000, 2016, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or
>>> modify it
>>> @@ -28,6 +28,7 @@
>>> package java.nio;
>>>
>>> import java.io.FileDescriptor;
>>> +import java.lang.ref.Reference;
>>> import jdk.internal.misc.VM;
>>> import jdk.internal.ref.Cleaner;
>>> import sun.nio.ch.DirectBuffer;
>>> @@ -312,6 +313,7 @@
>>> public $Type$Buffer put($type$ x) {
>>> #if[rw]
>>> UNSAFE.put$Swaptype$(ix(nextPutIndex()), $swap$($toBits$(x)));
>>> + Reference.reachabilityFence(this);
>>> return this;
>>> #else[rw]
>>> throw new ReadOnlyBufferException();
>>> @@ -321,6 +323,7 @@
>>> public $Type$Buffer put(int i, $type$ x) {
>>> #if[rw]
>>> UNSAFE.put$Swaptype$(ix(checkIndex(i)), $swap$($toBits$(x)));
>>> + Reference.reachabilityFence(this);
>>> return this;
>>> #else[rw]
>>> throw new ReadOnlyBufferException();
>>> @@ -347,6 +350,7 @@
>>> if (srem > rem)
>>> throw new BufferOverflowException();
>>> UNSAFE.copyMemory(sb.ix(spos), ix(pos), (long)srem <<
>>> $LG_BYTES_PER_VALUE$);
>>> + Reference.reachabilityFence(this);
>>> sb.position(spos + srem);
>>> position(pos + srem);
>>> } else if (src.hb != null) {
>>> @@ -413,6 +417,7 @@
>>> int rem = (pos <= lim ? lim - pos : 0);
>>>
>>> UNSAFE.copyMemory(ix(pos), ix(0), (long)rem <<
>>> $LG_BYTES_PER_VALUE$);
>>> + Reference.reachabilityFence(this);
>>> position(rem);
>>> limit(capacity());
>>> discardMark();
>>> @@ -505,6 +510,7 @@
>>> void _put(int i, byte b) { // package-private
>>> #if[rw]
>>> UNSAFE.putByte(address + i, b);
>>> + Reference.reachabilityFence(this);
>>> #else[rw]
>>> throw new ReadOnlyBufferException();
>>> #end[rw]
>>> diff -r d51e64840b4f
>>> src/java.base/share/classes/java/nio/MappedByteBuffer.java
>>> --- a/src/java.base/share/classes/java/nio/MappedByteBuffer.java Wed
>>> Jan
>>> 31 12:04:53 2018 +0800
>>> +++ b/src/java.base/share/classes/java/nio/MappedByteBuffer.java Thu
>>> Feb
>>> 01 11:30:10 2018 +0000
>>> @@ -1,5 +1,5 @@
>>> /*
>>> - * Copyright (c) 2000, 2013, Oracle and/or its affiliates. All rights
>>> reserved.
>>> + * Copyright (c) 2000, 2018, Oracle and/or its affiliates. All rights
>>> reserved.
>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>> *
>>> * This code is free software; you can redistribute it and/or
>>> modify it
>>> @@ -26,6 +26,7 @@
>>> package java.nio;
>>>
>>> import java.io.FileDescriptor;
>>> +import java.lang.ref.Reference;
>>> import jdk.internal.misc.Unsafe;
>>>
>>>
>>> @@ -164,6 +165,7 @@
>>> // is computed as we go along to prevent the compiler from
>>> otherwise
>>> // considering the loop as dead code.
>>> Unsafe unsafe = Unsafe.getUnsafe();
>>> + Reference.reachabilityFence(this);
>>> int ps = Bits.pageSize();
>>> int count = Bits.pageCount(length);
>>> long a = mappingAddress(offset);
>>> -------------------------------------------------------------------------------------------------------------------
>>>
>>>
>>>
>>> Regards,
>>> Ben Walsh
>>>
>>>
>>> Unless stated otherwise above:
>>> IBM United Kingdom Limited - Registered in England and Wales with
>>> number
>>> 741598.
>>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
>>> PO6 3AU
>>>
>
>
More information about the core-libs-dev
mailing list