Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 01/25/2016 05:14 PM, Alan Bateman wrote: On 25/01/2016 15:27, Peter Levart wrote: But let me ask something. Doesn't GC processing require (at least in some phases) that user threads be stopped in a safepoint? What happens when a user thread is blocked by kernel on memory access? Such thread can't be stopped in a safepoint. Safepoint can't begin, so GC can't proceed and therefore can't promote objects to old generation at that time. Am I right? If yes, then time does not pass for objects while a user thread is blocked on memory access, so they won't get promoted to old gen any time sooner after the user thread is unblocked. With memory mapping then it's very possible that a memory access to stall waiting for pages to be faulted in. So yes, I assume any STW/safepoint is going to be stall. On the other hand then threads doing file or socket I/O on these buffers will be in native (maybe blocked, maybe not) and so aren't an issue. They will safepoint on return or if they attempt to re-enter. -Alan Ah, yes. I/O with direct buffers. All the places that use sun.nio.ch.DirectBuffer.address() to obtain address of DB and then access the memory directly would also have to obtain current guard from DB and keep it until after the last access to the memory. There are 28 such usages in JDK. Usages with blocking I/O could use a different strategy. They could use reference counting directly (invoke increment/decrement on the Deallocator's guardCount and deallocate when it drops to 0). I/O is usually invoked for bulk transfers, so adding an atomic increment and decrement of an int variable would not present too much overhead for such usages, I think. Regards, Peter
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi,
On 01/25/2016 08:32 PM, Gil Tene wrote:
I assume your goal here is to get the resources released with the next newgen
collections (following a close()), rather than wait for an oldgen (if the
resource was held by an old object). That's a cool thing.
With that in mind, you can replace the repeated periodic
polling/flipping/allocation and external calling changeGuard() with a simple
internal GC-detecor that would call changeGuard() allocate a new guard only
once per newgen GC cycle.
This can take the form of adding a simple GCDetector inside your implementation:
private class GCDetector {
@Override
protected void finalize() throws Throwable {
GCDetector detector = new GCDetector();
changeGuard();
Reference.reachabilityFence(detector);
}
}
// The reason to use finalize here instead of a phantom ref
// based cleaner is that it would trigger immediately after the cycle,
// rather than potentially take an extra cycle to trigger.
// This can be done with a weakRef based cleaner instead
// (but probably when one is added to the JDK, otherwise you'd
// need your own polling thread and logic here...).
Good idea. This will adapt the rate of guard changing to the rate of GC
and there's no need for special executor. A phantom-ref cleaner would be
equally prompt as finalize(). When there's a finalze() method on the
referent, finalize() method is invoked 1st, then the FinalReference to
the referent is cleared and only after that the PhantomReference to the
same referent can be processed in the next GC cycle. But when there's no
final() method on the referent, PhantomReference is processed right away.
Also, when a finalize() method creates new finalizable object that has a
finalize() method which creates new finalizable object that has a
finalize() method ...
public class Test {
static class GCDetector {
@Override
protected void finalize() throws Throwable {
System.out.println("GC detected: " + this);
new GCDetector();
}
}
public static void main(String[] args) throws Exception {
System.runFinalizersOnExit(true);
new GCDetector();
}
}
...and you set to run finalizers on exit, you get a never-ending loop
and VM doesn't exit:
...
GC detected: Test$GCDetector@1088249a
GC detected: Test$GCDetector@7e73ecc
GC detected: Test$GCDetector@4da49a96
GC detected: Test$GCDetector@667a971f
GC detected: Test$GCDetector@3787d3be
...
So a phantom-ref cleaner would work better here. The function of
Deallocator and guard-changer can even be merged in the same object, so
guard-changing and reference tracking can be performed by the same
Cleaner(s) like in:
http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/v2/CloseableMemory.java
Regards, Peter
You'll need to allocate a single instance of GCDetector during construction of a
CloseableMemory, without retaining a reference to it after construction. This will start
a finalize-triggering chain per instance, with the chain "ticking" once per
newgen cycle.
If you want to avoid having one of these (coming and going on each GC cycle)
per CloseableMemory instance, you can use a common static detector and a
registration mechanism (where each registered instance would have it's
changeGuard() method call…
— Gil.
On Jan 24, 2016, at 9:10 AM, Peter Levart wrote:
Hi,
I had an idea recently on how to expedite the collection of an object. It is
simple - just don't let it live long.
Here's a concept prototype:
http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/CloseableMemory.java
The overhead of the check in access methods (getByte()/setByte()) amounts to
one volatile read of an oop variable that changes once per say 5 to 10 seconds.
That's the period a special guard object is alive. It's reachability is tracked
by the GC and extends to the end of each access method (using
Reference.reachabilityFence). Every few seconds, the guard object is changed
with new fresh one so that the chance of the guard and its tracking Cleaner
being promoted to old generation is very low.
Could something like that enable a low-overhead CloseableMappedByteBuffer?
Regards, Peter
On 01/23/2016 09:31 PM, Andrew Haley wrote:
On 23/01/16 20:01, Uwe Schindler wrote:
It depends how small! If the speed is still somewhere between Java 8
ByteBuffer performance and the recent Hotspot improvements in Java
9, I agree with trying it out. But some volatile memory access on
every access is a no-go. The code around ByteBufferIndexInput in
Lucene is the most performance-critical critical code, because on
every search query or sorting there is all the work happening in
there (millions of iterations with positional ByteBuffer.get*
calls). As ByteBuffers are limited to 2 GiB, we also need lots of
hairy code to work around that limitation!
Yes, I see that code. It would be helpful if there were a
self-contained but realistic benchmark using
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Uwe, On 28 Jan 2016, at 11:28, Uwe Schindler wrote: > Hi, > > Could you update the JIRA issue > (https://bugs.openjdk.java.net/browse/JDK-8132928) and JEP 260 > (http://openjdk.java.net/jeps/260) about the change we discussed here > (jdk.internal.ref.Cleaner now implements Runable to support Lucene and > similar apps needing to hack into cleaner), mainly in the section "open > issues”? You are still in unsupported waters. You are using reflection and setAccessible(true) to gain access to the internals of direct buffer. The refactoring we are doing in Cleaner as part of this issue, has a positive side-effect, from a library maintainers point of view ( less reflective code and static usage of internal types), but this is not to be confused with an agreed upon supported interface. It is just to keep things working, as we refactor / move some internal types in the JDK. Longer term a standard Java SE API should replace this. JEP 260 will, for the present time, continue to list sun.misc.Cleaner as an open issue, until we are satisfied that there is no critical need for it, outside of the direct/mapped buffer usage. > Maybe also add a comment to the source at the "@Override public void run()" > decl, so it may not get lost by later refactorings (if somebody thinks > "WTF!?") > > The update of above docs would be fine for us to have some reference point to > cite when we implement the changed Hack. BTW, we already have a preliminary > patch for Lucene (untested): > https://issues.apache.org/jira/secure/attachment/12784516/LUCENE-6989.patch Thanks for doing this Uwe. I really appreciate your feedback and prompt replies on this topic. -Chris. > Thanks for taking care of this issue. > Uwe > > - > Uwe Schindler > uschind...@apache.org > ASF Member, Apache Lucene PMC / Committer > Bremen, Germany > http://lucene.apache.org/ > >> -Original Message- >> From: Uwe Schindler [mailto:uschind...@apache.org] >> Sent: Tuesday, January 26, 2016 5:49 PM >> To: 'Chris Hegarty' ; 'Alan Bateman' >> ; 'Roger Riggs' ; >> uschind...@apache.org >> Cc: 'core-libs-dev' >> Subject: RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref >> >> Hi, >> >> API changes l and security check look good to me. I don't have time to >> compile and test a JDK, but I trust you that it works :-) >> >> Uwe >> >> - >> Uwe Schindler >> uschind...@apache.org >> ASF Member, Apache Lucene PMC / Committer >> Bremen, Germany >> http://lucene.apache.org/ >> >>> -Original Message- >>> From: Chris Hegarty [mailto:chris.hega...@oracle.com] >>> Sent: Tuesday, January 26, 2016 5:28 PM >>> To: Alan Bateman ; Roger Riggs >>> ; uschind...@apache.org >>> Cc: core-libs-dev >>> Subject: Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref >>> >>> Latest webrev updated in-place: >>> http://cr.openjdk.java.net/~chegar/8148117/ >>> >>> * to execute the run method requires an appropriate permission >>> * reverted any copyright changes ( leave to a bulk update ) >>> * updated the test to remove the script >>> >>> -Chris. >>> >>> >>> On 26 Jan 2016, at 15:23, Alan Bateman >> wrote: >>> >>>> On 26/01/2016 13:55, Chris Hegarty wrote: >>>>> It is wonderful to see the various ideas on this thread about the longer >>>>> term solution to the prompt releasing of direct buffer native memory. I >>>>> do not want to obstruct that ( it is very informative ), but I’d like to >>>>> warp >>> up >>>>> the review on the actual moving of Cleaner. To that end, I’ve update the >>>>> webrev as per Alan’s comments and suggestion ( to extend Runnable ). >>>>> >>>>> http://cr.openjdk.java.net/~chegar/8148117/ >>>>> >>>>> -Chris. >>>>> >>>>> >>>> This looks okay. As a defensive-in-depth then Cleaner::run can do a >>> permission check and should ease concerns about leakage. >>> > >
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, Could you update the JIRA issue (https://bugs.openjdk.java.net/browse/JDK-8132928) and JEP 260 (http://openjdk.java.net/jeps/260) about the change we discussed here (jdk.internal.ref.Cleaner now implements Runable to support Lucene and similar apps needing to hack into cleaner), mainly in the section "open issues"? Maybe also add a comment to the source at the "@Override public void run()" decl, so it may not get lost by later refactorings (if somebody thinks "WTF!?") The update of above docs would be fine for us to have some reference point to cite when we implement the changed Hack. BTW, we already have a preliminary patch for Lucene (untested): https://issues.apache.org/jira/secure/attachment/12784516/LUCENE-6989.patch Thanks for taking care of this issue. Uwe - Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ > -Original Message- > From: Uwe Schindler [mailto:uschind...@apache.org] > Sent: Tuesday, January 26, 2016 5:49 PM > To: 'Chris Hegarty' ; 'Alan Bateman' > ; 'Roger Riggs' ; > uschind...@apache.org > Cc: 'core-libs-dev' > Subject: RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > Hi, > > API changes l and security check look good to me. I don't have time to > compile and test a JDK, but I trust you that it works :-) > > Uwe > > - > Uwe Schindler > uschind...@apache.org > ASF Member, Apache Lucene PMC / Committer > Bremen, Germany > http://lucene.apache.org/ > > > -Original Message- > > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > > Sent: Tuesday, January 26, 2016 5:28 PM > > To: Alan Bateman ; Roger Riggs > > ; uschind...@apache.org > > Cc: core-libs-dev > > Subject: Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > > > Latest webrev updated in-place: > > http://cr.openjdk.java.net/~chegar/8148117/ > > > > * to execute the run method requires an appropriate permission > > * reverted any copyright changes ( leave to a bulk update ) > > * updated the test to remove the script > > > > -Chris. > > > > > > On 26 Jan 2016, at 15:23, Alan Bateman > wrote: > > > > > On 26/01/2016 13:55, Chris Hegarty wrote: > > >> It is wonderful to see the various ideas on this thread about the longer > > >> term solution to the prompt releasing of direct buffer native memory. I > > >> do not want to obstruct that ( it is very informative ), but I’d like to > > >> warp > > up > > >> the review on the actual moving of Cleaner. To that end, I’ve update the > > >> webrev as per Alan’s comments and suggestion ( to extend Runnable ). > > >> > > >> http://cr.openjdk.java.net/~chegar/8148117/ > > >> > > >> -Chris. > > >> > > >> > > > This looks okay. As a defensive-in-depth then Cleaner::run can do a > > permission check and should ease concerns about leakage. > >
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
> On Jan 26, 2016, at 8:27 AM, Chris Hegarty wrote: > > Latest webrev updated in-place: > http://cr.openjdk.java.net/~chegar/8148117/ > > * to execute the run method requires an appropriate permission > * reverted any copyright changes ( leave to a bulk update ) > * updated the test to remove the script Catching up on this review thread. The patch looks good. Having jdk.internal.ref.Cleaner to implement Runnable with the permission check looks reasonable. Mandy
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 26/01/2016 16:27, Chris Hegarty wrote: Latest webrev updated in-place: http://cr.openjdk.java.net/~chegar/8148117/ * to execute the run method requires an appropriate permission * reverted any copyright changes ( leave to a bulk update ) * updated the test to remove the script -Chris. This version looks good to me. -Alan.
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, API changes l and security check look good to me. I don't have time to compile and test a JDK, but I trust you that it works :-) Uwe - Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Tuesday, January 26, 2016 5:28 PM > To: Alan Bateman ; Roger Riggs > ; uschind...@apache.org > Cc: core-libs-dev > Subject: Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > Latest webrev updated in-place: > http://cr.openjdk.java.net/~chegar/8148117/ > > * to execute the run method requires an appropriate permission > * reverted any copyright changes ( leave to a bulk update ) > * updated the test to remove the script > > -Chris. > > > On 26 Jan 2016, at 15:23, Alan Bateman wrote: > > > On 26/01/2016 13:55, Chris Hegarty wrote: > >> It is wonderful to see the various ideas on this thread about the longer > >> term solution to the prompt releasing of direct buffer native memory. I > >> do not want to obstruct that ( it is very informative ), but I’d like to > >> warp > up > >> the review on the actual moving of Cleaner. To that end, I’ve update the > >> webrev as per Alan’s comments and suggestion ( to extend Runnable ). > >> > >> http://cr.openjdk.java.net/~chegar/8148117/ > >> > >> -Chris. > >> > >> > > This looks okay. As a defensive-in-depth then Cleaner::run can do a > permission check and should ease concerns about leakage. >
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Latest webrev updated in-place: http://cr.openjdk.java.net/~chegar/8148117/ * to execute the run method requires an appropriate permission * reverted any copyright changes ( leave to a bulk update ) * updated the test to remove the script -Chris. On 26 Jan 2016, at 15:23, Alan Bateman wrote: > On 26/01/2016 13:55, Chris Hegarty wrote: >> It is wonderful to see the various ideas on this thread about the longer >> term solution to the prompt releasing of direct buffer native memory. I >> do not want to obstruct that ( it is very informative ), but I’d like to >> warp up >> the review on the actual moving of Cleaner. To that end, I’ve update the >> webrev as per Alan’s comments and suggestion ( to extend Runnable ). >> >> http://cr.openjdk.java.net/~chegar/8148117/ >> >> -Chris. >> >> > This looks okay. As a defensive-in-depth then Cleaner::run can do a > permission check and should ease concerns about leakage.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 26 Jan 2016, at 16:36, Roger Riggs wrote: > Hi Chris, > > Looks good, thanks for updating the test. > > One typo: > "Unexpected exist > code” D’oh. Fixed in-place. -Chris > Roger > > > > On 1/26/2016 11:27 AM, Chris Hegarty wrote: >> Latest webrev updated in-place: >> >> http://cr.openjdk.java.net/~chegar/8148117/ >> >> >> * to execute the run method requires an appropriate permission >> * reverted any copyright changes ( leave to a bulk update ) >> * updated the test to remove the script >> >> -Chris. >> >> >> On 26 Jan 2016, at 15:23, Alan Bateman >> >> wrote: >> >> >>> On 26/01/2016 13:55, Chris Hegarty wrote: >>> It is wonderful to see the various ideas on this thread about the longer term solution to the prompt releasing of direct buffer native memory. I do not want to obstruct that ( it is very informative ), but I’d like to warp up the review on the actual moving of Cleaner. To that end, I’ve update the webrev as per Alan’s comments and suggestion ( to extend Runnable ). http://cr.openjdk.java.net/~chegar/8148117/ -Chris. >>> This looks okay. As a defensive-in-depth then Cleaner::run can do a >>> permission check and should ease concerns about leakage. >>> >> >> >> >
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi Chris, Looks good, thanks for updating the test. One typo: "Unexpected *exist *code" Roger On 1/26/2016 11:27 AM, Chris Hegarty wrote: Latest webrev updated in-place: http://cr.openjdk.java.net/~chegar/8148117/ * to execute the run method requires an appropriate permission * reverted any copyright changes ( leave to a bulk update ) * updated the test to remove the script -Chris. On 26 Jan 2016, at 15:23, Alan Bateman wrote: On 26/01/2016 13:55, Chris Hegarty wrote: It is wonderful to see the various ideas on this thread about the longer term solution to the prompt releasing of direct buffer native memory. I do not want to obstruct that ( it is very informative ), but I’d like to warp up the review on the actual moving of Cleaner. To that end, I’ve update the webrev as per Alan’s comments and suggestion ( to extend Runnable ). http://cr.openjdk.java.net/~chegar/8148117/ -Chris. This looks okay. As a defensive-in-depth then Cleaner::run can do a permission check and should ease concerns about leakage.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 26/01/2016 13:55, Chris Hegarty wrote: It is wonderful to see the various ideas on this thread about the longer term solution to the prompt releasing of direct buffer native memory. I do not want to obstruct that ( it is very informative ), but I’d like to warp up the review on the actual moving of Cleaner. To that end, I’ve update the webrev as per Alan’s comments and suggestion ( to extend Runnable ). http://cr.openjdk.java.net/~chegar/8148117/ -Chris. This looks okay. As a defensive-in-depth then Cleaner::run can do a permission check and should ease concerns about leakage. -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi Chris, Looks fine. Perhaps update the copyrights. Upgrading the shell based test to java based would be good sometime too. Also, there is a more recent version of webrev [1] that provides convenient next and previous links. Thanks, Roger [1] http://hg.openjdk.java.net/code-tools/webrev/raw-file/tip/webrev.ksh On 1/26/2016 8:55 AM, Chris Hegarty wrote: It is wonderful to see the various ideas on this thread about the longer term solution to the prompt releasing of direct buffer native memory. I do not want to obstruct that ( it is very informative ), but I’d like to warp up the review on the actual moving of Cleaner. To that end, I’ve update the webrev as per Alan’s comments and suggestion ( to extend Runnable ). http://cr.openjdk.java.net/~chegar/8148117/ -Chris. On 23 Jan 2016, at 16:36, Alan Bateman wrote: On 23/01/2016 16:16, Chris Hegarty wrote: : Webrev: http://cr.openjdk.java.net/~chegar/8148117/ This has to move to your patch looks okay. You might need to update the TEST.groups to ensure that the existOnThrow tests will be run by jdk_core. -Alan.
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi,
> On 26/01/2016 14:05, Uwe Schindler wrote:
> > Hi,
> >
> > looks good to me. Once we have EA builds with that I will adapt the
> ByteBufferIndexInput code in Lucene.
> >
> > One thing about the Runable: This should work perfectly fine, because we
> only need to make call the getCleaner() method accessible, call it and finally
> check if return type implements Runable (if not we fall back to pre Java 9
> code). We can then cast and invoke the cleaner without any further
> reflection.
> >
> > But there is now some security related issue: The reflection on DirectBuffer
> should work in most environments (also with security managers), so you can
> get the Cleaner instance with a doPrivileged (ideally). But as this cleaner
> instance was previously inside sun.misc package, and you needed
> RuntimePermission "accessInPackage.sun.misc" to call and make the clean
> method accessible. But with the new patch you no longer need this
> permission, making it easier to invoke that method.
> >
> This is a private field so if you are hacking into it then you'll need
> ReflectPermission("suppressAccessChecks") when running with a security
> manager. Clearly anything with a reference to a Cleaner needs to be
> super careful not to let it leak to untrusted code.
I know and Lucene takes care of that.
I just said, previously you needed both runtime permissions:
"accessInPackage.sun.misc" AND "suppressAccessChecks", now only one. I just
suggested to add the 2nd hurdle back somehow, as additional safety.
Otherwise I am fine with the patch, was just meant as "improvement". E.g.,
Elasticsearch already gives *only* lucene-core.jar both permisisions, nothing
else in its code!!!
Uwe
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 26/01/2016 14:05, Uwe Schindler wrote:
Hi,
looks good to me. Once we have EA builds with that I will adapt the
ByteBufferIndexInput code in Lucene.
One thing about the Runable: This should work perfectly fine, because we only
need to make call the getCleaner() method accessible, call it and finally check
if return type implements Runable (if not we fall back to pre Java 9 code). We
can then cast and invoke the cleaner without any further reflection.
But there is now some security related issue: The reflection on DirectBuffer should work
in most environments (also with security managers), so you can get the Cleaner instance
with a doPrivileged (ideally). But as this cleaner instance was previously inside
sun.misc package, and you needed RuntimePermission "accessInPackage.sun.misc"
to call and make the clean method accessible. But with the new patch you no longer need
this permission, making it easier to invoke that method.
This is a private field so if you are hacking into it then you'll need
ReflectPermission("suppressAccessChecks") when running with a security
manager. Clearly anything with a reference to a Cleaner needs to be
super careful not to let it leak to untrusted code.
-Alan
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, looks good to me. Once we have EA builds with that I will adapt the ByteBufferIndexInput code in Lucene. One thing about the Runable: This should work perfectly fine, because we only need to make call the getCleaner() method accessible, call it and finally check if return type implements Runable (if not we fall back to pre Java 9 code). We can then cast and invoke the cleaner without any further reflection. But there is now some security related issue: The reflection on DirectBuffer should work in most environments (also with security managers), so you can get the Cleaner instance with a doPrivileged (ideally). But as this cleaner instance was previously inside sun.misc package, and you needed RuntimePermission "accessInPackage.sun.misc" to call and make the clean method accessible. But with the new patch you no longer need this permission, making it easier to invoke that method. In my opinion, you should add some other runtime permission for safety, which is checked on calling the run() method. This would make it safe, because as soon as you have SecurityManager you would need to give explicit permission to the code. Uwe - Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ > -Original Message- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On > Behalf Of Chris Hegarty > Sent: Tuesday, January 26, 2016 2:56 PM > To: Alan Bateman ; core-libs-dev d...@openjdk.java.net> > Subject: Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > It is wonderful to see the various ideas on this thread about the longer > term solution to the prompt releasing of direct buffer native memory. I > do not want to obstruct that ( it is very informative ), but I’d like to warp > up > the review on the actual moving of Cleaner. To that end, I’ve update the > webrev as per Alan’s comments and suggestion ( to extend Runnable ). > > http://cr.openjdk.java.net/~chegar/8148117/ > > -Chris. > > On 23 Jan 2016, at 16:36, Alan Bateman wrote: > > > On 23/01/2016 16:16, Chris Hegarty wrote: > >> : > >> > >> Webrev: > >> http://cr.openjdk.java.net/~chegar/8148117/ > >> > >> > > This has to move to your patch looks okay. You might need to update the > TEST.groups to ensure that the existOnThrow tests will be run by jdk_core. > > > > -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
> On 23 Jan 2016, at 21:31, Andrew Haley wrote: > > BTW, does anyone here know why we don't have humongous ByteBuffers > with a long index? > Probably in part because of Java arrays. IMO we need Arrays 2.0 (+ panama i suspect). Here’s some "shoot from the hip" thinking… In principle a direct ByteBuffer could bound a region larger than that which can be expressed with an int limit (i suppose a heap ByteBuffer could do the same if it holds multiple instances if byte[], although that would require more work). The direct BB can be used to access the first 2^31 - 1 values. A VarHandle could be produced that accesses the whole range since it could accept long index values and check bounds against a special long limit field. That might require some additional HotSpot work to optimize bounds checks and any loops to reduce or avoid a safe point check per iteration. Paul.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
It is wonderful to see the various ideas on this thread about the longer term solution to the prompt releasing of direct buffer native memory. I do not want to obstruct that ( it is very informative ), but I’d like to warp up the review on the actual moving of Cleaner. To that end, I’ve update the webrev as per Alan’s comments and suggestion ( to extend Runnable ). http://cr.openjdk.java.net/~chegar/8148117/ -Chris. On 23 Jan 2016, at 16:36, Alan Bateman wrote: > On 23/01/2016 16:16, Chris Hegarty wrote: >> : >> >> Webrev: >> http://cr.openjdk.java.net/~chegar/8148117/ >> >> > This has to move to your patch looks okay. You might need to update the > TEST.groups to ensure that the existOnThrow tests will be run by jdk_core. > > -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
I assume your goal here is to get the resources released with the next newgen
collections (following a close()), rather than wait for an oldgen (if the
resource was held by an old object). That's a cool thing.
With that in mind, you can replace the repeated periodic
polling/flipping/allocation and external calling changeGuard() with a simple
internal GC-detecor that would call changeGuard() allocate a new guard only
once per newgen GC cycle.
This can take the form of adding a simple GCDetector inside your implementation:
private class GCDetector {
@Override
protected void finalize() throws Throwable {
GCDetector detector = new GCDetector();
changeGuard();
Reference.reachabilityFence(detector);
}
}
// The reason to use finalize here instead of a phantom ref
// based cleaner is that it would trigger immediately after the cycle,
// rather than potentially take an extra cycle to trigger.
// This can be done with a weakRef based cleaner instead
// (but probably when one is added to the JDK, otherwise you'd
// need your own polling thread and logic here...).
You'll need to allocate a single instance of GCDetector during construction of
a CloseableMemory, without retaining a reference to it after construction. This
will start a finalize-triggering chain per instance, with the chain "ticking"
once per newgen cycle.
If you want to avoid having one of these (coming and going on each GC cycle)
per CloseableMemory instance, you can use a common static detector and a
registration mechanism (where each registered instance would have it's
changeGuard() method call…
— Gil.
> On Jan 24, 2016, at 9:10 AM, Peter Levart wrote:
>
> Hi,
>
> I had an idea recently on how to expedite the collection of an object. It is
> simple - just don't let it live long.
>
> Here's a concept prototype:
>
> http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/CloseableMemory.java
>
> The overhead of the check in access methods (getByte()/setByte()) amounts to
> one volatile read of an oop variable that changes once per say 5 to 10
> seconds. That's the period a special guard object is alive. It's reachability
> is tracked by the GC and extends to the end of each access method (using
> Reference.reachabilityFence). Every few seconds, the guard object is changed
> with new fresh one so that the chance of the guard and its tracking Cleaner
> being promoted to old generation is very low.
>
> Could something like that enable a low-overhead CloseableMappedByteBuffer?
>
> Regards, Peter
>
> On 01/23/2016 09:31 PM, Andrew Haley wrote:
>> On 23/01/16 20:01, Uwe Schindler wrote:
>>
>>> It depends how small! If the speed is still somewhere between Java 8
>>> ByteBuffer performance and the recent Hotspot improvements in Java
>>> 9, I agree with trying it out. But some volatile memory access on
>>> every access is a no-go. The code around ByteBufferIndexInput in
>>> Lucene is the most performance-critical critical code, because on
>>> every search query or sorting there is all the work happening in
>>> there (millions of iterations with positional ByteBuffer.get*
>>> calls). As ByteBuffers are limited to 2 GiB, we also need lots of
>>> hairy code to work around that limitation!
>> Yes, I see that code. It would be helpful if there were a
>> self-contained but realistic benchmark using that code. That way,
>> some simple experiments would allow changes to be measured.
>>
>>> If you look at ByteBufferIndexInput's code you will see that we
>>> simply do stuff like trying to read from one bytebuffer and only if
>>> we catch an BufferUnderflowException we fall back to handling buffer
>>> switches: Instead of checking bounds on every access, we have
>>> fallback code only happening on exceptions. E.g. if you are 3 bytes
>>> before end of one buffer slice and read a long, it will throw
>>> BufferUnderflow. When this happens the code will fall back to read
>>> byte by byte from 2 different buffers and reassemble the long):
>> I'm surprised you don't see painful deoptimization traps when that
>> happens. I suppose it's rare enough that you don't care. There's a
>> new group of methods in JDK9 called Objects.checkIndex() which are
>> intended to provide a very efficient way to do bounds checks. It might
>> be interesting to see if they work well with ByteBufferIndexInput:
>> that's an important use case.
>>
>> BTW, does anyone here know why we don't have humongous ByteBuffers
>> with a long index?
>>
>> Andrew.
>
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 25/01/2016 15:27, Peter Levart wrote: But let me ask something. Doesn't GC processing require (at least in some phases) that user threads be stopped in a safepoint? What happens when a user thread is blocked by kernel on memory access? Such thread can't be stopped in a safepoint. Safepoint can't begin, so GC can't proceed and therefore can't promote objects to old generation at that time. Am I right? If yes, then time does not pass for objects while a user thread is blocked on memory access, so they won't get promoted to old gen any time sooner after the user thread is unblocked. With memory mapping then it's very possible that a memory access to stall waiting for pages to be faulted in. So yes, I assume any STW/safepoint is going to be stall. On the other hand then threads doing file or socket I/O on these buffers will be in native (maybe blocked, maybe not) and so aren't an issue. They will safepoint on return or if they attempt to re-enter. -Alan
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 01/25/2016 02:31 PM, Alan Bateman wrote: On 24/01/2016 17:10, Peter Levart wrote: Hi, I had an idea recently on how to expedite the collection of an object. It is simple - just don't let it live long. Here's a concept prototype: http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/CloseableMemory.java The overhead of the check in access methods (getByte()/setByte()) amounts to one volatile read of an oop variable that changes once per say 5 to 10 seconds. That's the period a special guard object is alive. It's reachability is tracked by the GC and extends to the end of each access method (using Reference.reachabilityFence). Every few seconds, the guard object is changed with new fresh one so that the chance of the guard and its tracking Cleaner being promoted to old generation is very low. Could something like that enable a low-overhead CloseableMappedByteBuffer? If I read this correctly then this still depends on timely reference processing. Right, the hope is that Reference(s) and referents that only live for a few seconds get this timely processing. The switching guard trick to keep the Cleaner from being promoted is interesting of course. So if we were to do something like this with MBB then it would mean the file mapping would still exist and so we'd have the usual issue of trying to delete the underlying file. There could be a method on MBB to register a callback to be called after the buffer is unmapped. Then we have the issue of file or socket I/O where threads could be blocked doing I/O with regions of the buffer and so delay indefinitely any release. Yes, that could be a problem. If the access to mapped memory blocked for a very long time, then the guard and its Cleaner could be promoted to old gen in the meanwhile. But let me ask something. Doesn't GC processing require (at least in some phases) that user threads be stopped in a safepoint? What happens when a user thread is blocked by kernel on memory access? Such thread can't be stopped in a safepoint. Safepoint can't begin, so GC can't proceed and therefore can't promote objects to old generation at that time. Am I right? If yes, then time does not pass for objects while a user thread is blocked on memory access, so they won't get promoted to old gen any time sooner after the user thread is unblocked. Regards, Peter That said, maybe it is an option for Uwe. -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 24/01/2016 17:10, Peter Levart wrote: Hi, I had an idea recently on how to expedite the collection of an object. It is simple - just don't let it live long. Here's a concept prototype: http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/CloseableMemory.java The overhead of the check in access methods (getByte()/setByte()) amounts to one volatile read of an oop variable that changes once per say 5 to 10 seconds. That's the period a special guard object is alive. It's reachability is tracked by the GC and extends to the end of each access method (using Reference.reachabilityFence). Every few seconds, the guard object is changed with new fresh one so that the chance of the guard and its tracking Cleaner being promoted to old generation is very low. Could something like that enable a low-overhead CloseableMappedByteBuffer? If I read this correctly then this still depends on timely reference processing. The switching guard trick to keep the Cleaner from being promoted is interesting of course. So if we were to do something like this with MBB then it would mean the file mapping would still exist and so we'd have the usual issue of trying to delete the underlying file. Then we have the issue of file or socket I/O where threads could be blocked doing I/O with regions of the buffer and so delay indefinitely any release. That said, maybe it is an option for Uwe. -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, I had an idea recently on how to expedite the collection of an object. It is simple - just don't let it live long. Here's a concept prototype: http://cr.openjdk.java.net/~plevart/misc/CloseableMemory/CloseableMemory.java The overhead of the check in access methods (getByte()/setByte()) amounts to one volatile read of an oop variable that changes once per say 5 to 10 seconds. That's the period a special guard object is alive. It's reachability is tracked by the GC and extends to the end of each access method (using Reference.reachabilityFence). Every few seconds, the guard object is changed with new fresh one so that the chance of the guard and its tracking Cleaner being promoted to old generation is very low. Could something like that enable a low-overhead CloseableMappedByteBuffer? Regards, Peter On 01/23/2016 09:31 PM, Andrew Haley wrote: On 23/01/16 20:01, Uwe Schindler wrote: It depends how small! If the speed is still somewhere between Java 8 ByteBuffer performance and the recent Hotspot improvements in Java 9, I agree with trying it out. But some volatile memory access on every access is a no-go. The code around ByteBufferIndexInput in Lucene is the most performance-critical critical code, because on every search query or sorting there is all the work happening in there (millions of iterations with positional ByteBuffer.get* calls). As ByteBuffers are limited to 2 GiB, we also need lots of hairy code to work around that limitation! Yes, I see that code. It would be helpful if there were a self-contained but realistic benchmark using that code. That way, some simple experiments would allow changes to be measured. If you look at ByteBufferIndexInput's code you will see that we simply do stuff like trying to read from one bytebuffer and only if we catch an BufferUnderflowException we fall back to handling buffer switches: Instead of checking bounds on every access, we have fallback code only happening on exceptions. E.g. if you are 3 bytes before end of one buffer slice and read a long, it will throw BufferUnderflow. When this happens the code will fall back to read byte by byte from 2 different buffers and reassemble the long): I'm surprised you don't see painful deoptimization traps when that happens. I suppose it's rare enough that you don't care. There's a new group of methods in JDK9 called Objects.checkIndex() which are intended to provide a very efficient way to do bounds checks. It might be interesting to see if they work well with ByteBufferIndexInput: that's an important use case. BTW, does anyone here know why we don't have humongous ByteBuffers with a long index? Andrew.
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi all, hi Chris, Thanks for the catch up! As we are continuously testing EA builds of OpenJDK with Apache Lucene, we will see the issues caused by this commit asap. So I would maybe delay our testing a bit, until the proposed changes of Alan are also committed and part of EA builds. Is there already an issue about that? I opened an issue in the Apache Lucene tracker: https://issues.apache.org/jira/browse/LUCENE-6989 I hope, we will find a good solution! I had some other thoughts about the issue: Basically the forceful unmapping is only needed because the Grabage Collector never ever tries to finalize and GC the very small MappedByteBuffer instances, especially if they were living for longer time. But they are sitting on heavy resources, so maybe a solution to solve this would be: - Add some special annotation in the JDK that is respected by all Garbage Collectors and makes them prefer those "heavy" instances. Something like @jdk.internal.Heavy - Maybe add some close() method to MappedByteBuffer, but just make it a candidate for earlier Grabage Collection instead of doing risky unmapping. A solution may be to add it to some special GC queue or whatever. I have no idea if that’s possible, just tell GC: "I am done with this object, discard it asap if its unreachable!" Uwe - Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ > -Original Message- > From: Chris Hegarty [mailto:chris.hega...@oracle.com] > Sent: Saturday, January 23, 2016 11:52 PM > To: Uwe Schindler > Cc: Alan Bateman ; core-libs-dev d...@openjdk.java.net> > Subject: Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > Just catching up… > > I see no objection to the proposed patch, but the obvious concern ( that > is highlighted in JEP 260 ), about the digging into the private internal > details of NIO buffers. > > I think Alan summarized the issues around providing a public API for > releasing/unmapping native memory very well, and if this does not > make it for JDK 9, then Alan’s alternative ( implements Runnable ) may > be a better interim solution than the current use of reflection. > > -Chris. > > > On 23 Jan 2016, at 21:57, Uwe Schindler wrote: > > > > Hi, > > > > Alan Bateman [mailto:alan.bate...@oracle.com] wrote: > >> On 23/01/2016 19:23, Uwe Schindler wrote: > >>> : > >>> > >>> What is the replacement for the following code (see marked > BufferCleaner > >> impl, which is our abstraction): https://goo.gl/DGYZZj > >>> > >>> The main reason why sun.misc.Cleaner was on the critical API list is NOT > the > >> case that somebody wanted to implement their own cleaner as > replacement > >> for finalization. The main reason (and this is *not* solved by the new APIs > in > >> java.lang.ref.Cleaner) is to *force* unmapping of direct and mmapped > byte > >> buffers. > >> JEP 260 was updated recently to move this to an open issue and I think > >> the text captures the issue correctly. Part of it that we can't have > >> types in java.base depending on types in other modules and this is why > >> Buffers can't have fields of type sun.misc.Cleaner or any type in sun.misc. > >> > >> As has come several times, there are huge security and reliability > >> issues that arise when releasing or unmapping memory that is still > >> accessible via reachable objects. Proposed solutions over the years have > >> traded performance or were limited to platforms where we could > >> atomically remap. JDK 9 may be the time to try yet again or else just > >> introduce a JDK-specific API to unmap that comes with a warning in 96pt > >> font. It might have to be disabled completely when running with a > >> security manager. Alternatively the burden of the current hack could be > >> reduced by having Cleaner implement Runnable with a run method that > >> invokes clean. That would avoid needing to cast to a JDK-internal type > >> and so avoiding needing to use -XaddExports to break encapsulation. > > > > We know this problem. Because of this, e.g. Elasticsearch runs with a > security manager and only allows lucene-core.jar access to do this reflection > on sun.misc package (using the correct permission, only given to JAR file). > > > > I was about to suggest the same thing: Maybe add a public API to byte > buffer, that is documented to throw SecurityException. Add a new > Runtime/WhateverPermission that can be enabled to work around that (e.g., > Elasticsearch would do this and allow this Permission only to > lucene-
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Just catching up… I see no objection to the proposed patch, but the obvious concern ( that is highlighted in JEP 260 ), about the digging into the private internal details of NIO buffers. I think Alan summarized the issues around providing a public API for releasing/unmapping native memory very well, and if this does not make it for JDK 9, then Alan’s alternative ( implements Runnable ) may be a better interim solution than the current use of reflection. -Chris. > On 23 Jan 2016, at 21:57, Uwe Schindler wrote: > > Hi, > > Alan Bateman [mailto:alan.bate...@oracle.com] wrote: >> On 23/01/2016 19:23, Uwe Schindler wrote: >>> : >>> >>> What is the replacement for the following code (see marked BufferCleaner >> impl, which is our abstraction): https://goo.gl/DGYZZj >>> >>> The main reason why sun.misc.Cleaner was on the critical API list is NOT the >> case that somebody wanted to implement their own cleaner as replacement >> for finalization. The main reason (and this is *not* solved by the new APIs >> in >> java.lang.ref.Cleaner) is to *force* unmapping of direct and mmapped byte >> buffers. >> JEP 260 was updated recently to move this to an open issue and I think >> the text captures the issue correctly. Part of it that we can't have >> types in java.base depending on types in other modules and this is why >> Buffers can't have fields of type sun.misc.Cleaner or any type in sun.misc. >> >> As has come several times, there are huge security and reliability >> issues that arise when releasing or unmapping memory that is still >> accessible via reachable objects. Proposed solutions over the years have >> traded performance or were limited to platforms where we could >> atomically remap. JDK 9 may be the time to try yet again or else just >> introduce a JDK-specific API to unmap that comes with a warning in 96pt >> font. It might have to be disabled completely when running with a >> security manager. Alternatively the burden of the current hack could be >> reduced by having Cleaner implement Runnable with a run method that >> invokes clean. That would avoid needing to cast to a JDK-internal type >> and so avoiding needing to use -XaddExports to break encapsulation. > > We know this problem. Because of this, e.g. Elasticsearch runs with a > security manager and only allows lucene-core.jar access to do this reflection > on sun.misc package (using the correct permission, only given to JAR file). > > I was about to suggest the same thing: Maybe add a public API to byte buffer, > that is documented to throw SecurityException. Add a new > Runtime/WhateverPermission that can be enabled to work around that (e.g., > Elasticsearch would do this and allow this Permission only to > lucene-core.jar). In addition add red and blinking warnings to this method :-) > > In any case, a possible solution was proposed by Andrew Haley (he had some > ideas in the past, I think we brought this up the last time a few months ago) > with a small slowdown, but still providing better ByteBuffer performance than > in Java 7 or Java 8. > > I would hope for a solution that installs a SIGSEGV signal handler in the > JDK, that translates any completely illegal access that would otherwise lead > to a crash of the JDK to some Java Exception. The security issues you also > mention do not apply for us (e.g., reading memory from another web > application in the same JVM), so a new permission for SecurityManager would > also help here. > > Uwe >
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi Andrew, Andrew Haley [mailto:a...@redhat.com] wrote: > On 23/01/16 20:01, Uwe Schindler wrote: > > > It depends how small! If the speed is still somewhere between Java 8 > > ByteBuffer performance and the recent Hotspot improvements in Java > > 9, I agree with trying it out. But some volatile memory access on > > every access is a no-go. The code around ByteBufferIndexInput in > > Lucene is the most performance-critical critical code, because on > > every search query or sorting there is all the work happening in > > there (millions of iterations with positional ByteBuffer.get* > > calls). As ByteBuffers are limited to 2 GiB, we also need lots of > > hairy code to work around that limitation! > > Yes, I see that code. It would be helpful if there were a > self-contained but realistic benchmark using that code. That way, > some simple experiments would allow changes to be measured. Unfortunately I am a bit occupied with preparing for FOSDEM and also we switched Lucene/Solr to GIT today, so I cannot write a benchmark today. I know that Robert Muir (I CC'ed him) did a lot of performance testing with the ByteBufferIndexInput, maybe we can work both on some isolated benchmark, that you can use for testing. But this may take a while. > > If you look at ByteBufferIndexInput's code you will see that we > > simply do stuff like trying to read from one bytebuffer and only if > > we catch an BufferUnderflowException we fall back to handling buffer > > switches: Instead of checking bounds on every access, we have > > fallback code only happening on exceptions. E.g. if you are 3 bytes > > before end of one buffer slice and read a long, it will throw > > BufferUnderflow. When this happens the code will fall back to read > > byte by byte from 2 different buffers and reassemble the long): > > I'm surprised you don't see painful deoptimization traps when that > happens. I suppose it's rare enough that you don't care. Exactly! Of course the accesses to the ByteBufferIndexInput are not really completely random. You always have accesses concentrated on places close (otherwise this would produce lots of swapping and I/O). As the size of the ByteBuffer (the chunkSizePower in this code) is 1 GiB (2^30) by default, the probability to catch that exception and cause deoptimization is very low. By the way it is not 2 GiB, because signed integers and maximum array size in Java don't allow to reach 2^31 (only 2^21 - 1, damn!). So we chunk the file into 1 GiB large ByteBuffers. > There's a > new group of methods in JDK9 called Objects.checkIndex() which are > intended to provide a very efficient way to do bounds checks. It might > be interesting to see if they work well with ByteBufferIndexInput: > that's an important use case. We can check for this. The main idea in this code is to not do similar bounds checks both in ByteBuffer and outside. After switching to the try/catch we improved the whole situation. We can try with Java 9, but using Objects.checkIndex() can at earliest be used once Lucene is on Java 9 (or with Multi-Version-JAR files with ByteBufferIndexInput alternate impl for Java 9+). > BTW, does anyone here know why we don't have humongous ByteBuffers > with a long index? I think 64 bit arrays were proposed for Java 10, so are ByteBuffers with longs (or VarHandles using longs). This is also an old and long going discussion. Uwe > Andrew. Thanks, Uwe
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, Alan Bateman [mailto:alan.bate...@oracle.com] wrote: > On 23/01/2016 19:23, Uwe Schindler wrote: > > : > > > > What is the replacement for the following code (see marked BufferCleaner > impl, which is our abstraction): https://goo.gl/DGYZZj > > > > The main reason why sun.misc.Cleaner was on the critical API list is NOT the > case that somebody wanted to implement their own cleaner as replacement > for finalization. The main reason (and this is *not* solved by the new APIs in > java.lang.ref.Cleaner) is to *force* unmapping of direct and mmapped byte > buffers. > JEP 260 was updated recently to move this to an open issue and I think > the text captures the issue correctly. Part of it that we can't have > types in java.base depending on types in other modules and this is why > Buffers can't have fields of type sun.misc.Cleaner or any type in sun.misc. > > As has come several times, there are huge security and reliability > issues that arise when releasing or unmapping memory that is still > accessible via reachable objects. Proposed solutions over the years have > traded performance or were limited to platforms where we could > atomically remap. JDK 9 may be the time to try yet again or else just > introduce a JDK-specific API to unmap that comes with a warning in 96pt > font. It might have to be disabled completely when running with a > security manager. Alternatively the burden of the current hack could be > reduced by having Cleaner implement Runnable with a run method that > invokes clean. That would avoid needing to cast to a JDK-internal type > and so avoiding needing to use -XaddExports to break encapsulation. We know this problem. Because of this, e.g. Elasticsearch runs with a security manager and only allows lucene-core.jar access to do this reflection on sun.misc package (using the correct permission, only given to JAR file). I was about to suggest the same thing: Maybe add a public API to byte buffer, that is documented to throw SecurityException. Add a new Runtime/WhateverPermission that can be enabled to work around that (e.g., Elasticsearch would do this and allow this Permission only to lucene-core.jar). In addition add red and blinking warnings to this method :-) In any case, a possible solution was proposed by Andrew Haley (he had some ideas in the past, I think we brought this up the last time a few months ago) with a small slowdown, but still providing better ByteBuffer performance than in Java 7 or Java 8. I would hope for a solution that installs a SIGSEGV signal handler in the JDK, that translates any completely illegal access that would otherwise lead to a crash of the JDK to some Java Exception. The security issues you also mention do not apply for us (e.g., reading memory from another web application in the same JVM), so a new permission for SecurityManager would also help here. Uwe
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 23/01/2016 19:23, Uwe Schindler wrote: : What is the replacement for the following code (see marked BufferCleaner impl, which is our abstraction): https://goo.gl/DGYZZj The main reason why sun.misc.Cleaner was on the critical API list is NOT the case that somebody wanted to implement their own cleaner as replacement for finalization. The main reason (and this is *not* solved by the new APIs in java.lang.ref.Cleaner) is to *force* unmapping of direct and mmapped byte buffers. JEP 260 was updated recently to move this to an open issue and I think the text captures the issue correctly. Part of it that we can't have types in java.base depending on types in other modules and this is why Buffers can't have fields of type sun.misc.Cleaner or any type in sun.misc. As has come several times, there are huge security and reliability issues that arise when releasing or unmapping memory that is still accessible via reachable objects. Proposed solutions over the years have traded performance or were limited to platforms where we could atomically remap. JDK 9 may be the time to try yet again or else just introduce a JDK-specific API to unmap that comes with a warning in 96pt font. It might have to be disabled completely when running with a security manager. Alternatively the burden of the current hack could be reduced by having Cleaner implement Runnable with a run method that invokes clean. That would avoid needing to cast to a JDK-internal type and so avoiding needing to use -XaddExports to break encapsulation. -Alan.
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 23/01/16 20:01, Uwe Schindler wrote: > It depends how small! If the speed is still somewhere between Java 8 > ByteBuffer performance and the recent Hotspot improvements in Java > 9, I agree with trying it out. But some volatile memory access on > every access is a no-go. The code around ByteBufferIndexInput in > Lucene is the most performance-critical critical code, because on > every search query or sorting there is all the work happening in > there (millions of iterations with positional ByteBuffer.get* > calls). As ByteBuffers are limited to 2 GiB, we also need lots of > hairy code to work around that limitation! Yes, I see that code. It would be helpful if there were a self-contained but realistic benchmark using that code. That way, some simple experiments would allow changes to be measured. > If you look at ByteBufferIndexInput's code you will see that we > simply do stuff like trying to read from one bytebuffer and only if > we catch an BufferUnderflowException we fall back to handling buffer > switches: Instead of checking bounds on every access, we have > fallback code only happening on exceptions. E.g. if you are 3 bytes > before end of one buffer slice and read a long, it will throw > BufferUnderflow. When this happens the code will fall back to read > byte by byte from 2 different buffers and reassemble the long): I'm surprised you don't see painful deoptimization traps when that happens. I suppose it's rare enough that you don't care. There's a new group of methods in JDK9 called Objects.checkIndex() which are intended to provide a very efficient way to do bounds checks. It might be interesting to see if they work well with ByteBufferIndexInput: that's an important use case. BTW, does anyone here know why we don't have humongous ByteBuffers with a long index? Andrew.
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Hi, > Here's a question for you: could you tolerate a closeable mapped > ByteBuffer which was somewhat slower to access? It is hard to make a > secure closeable mapped ByteBuffer: the question is how small the > overhead of closeability can be made. Lucene uses the ByteBuffer like a conventional byte[] arrays and expects as close performance to that as possible. In Java 7 and Java 8 there is for sure some large overhead because of missing Hotspot optimizations. In Java 9 we found out, that recent builds are now almost as fast as byte[] access. Completely destroying that improvement would be no good. :-( We are also looking into using VarHandles instead of ByteBuffers directly (using the new MethodHandles methods to get a VarHandle on top of any ByteBuffer, although this does not really fit our use-case completely, because we cannot use views like long[] on top of a ByteBuffer, because we sometimes also need unaligned accesses). We will try to check this out as soon as first builds with VarHandles are available. Of course this would also not help, if a Closeable ByteBuffer would also slowdown the VarHandle accesses. > But if the answer is "We can't tolerate *any* additional overhead, no > matter how small" then there is no motivation to do any experiments. It depends how small! If the speed is still somewhere between Java 8 ByteBuffer performance and the recent Hotspot improvements in Java 9, I agree with trying it out. But some volatile memory access on every access is a no-go. The code around ByteBufferIndexInput in Lucene is the most performance-critical critical code, because on every search query or sorting there is all the work happening in there (millions of iterations with positional ByteBuffer.get* calls). As ByteBuffers are limited to 2 GiB, we also need lots of hairy code to work around that limitation! If you look at ByteBufferIndexInput's code you will see that we simply do stuff like trying to read from one bytebuffer and only if we catch an BufferUnderflowException we fall back to handling buffer switches: Instead of checking bounds on every access, we have fallback code only happening on exceptions. E.g. if you are 3 bytes before end of one buffer slice and read a long, it will throw BufferUnderflow. When this happens the code will fall back to read byte by byte from 2 different buffers and reassemble the long): https://goo.gl/g1jKcm Stuff like sorting will make heavy use of the ByteBufferIndexInput's (RandomAccessIndexInput interface's) methods like readXxx(long position) for stuff like quicksort with really hundreds of millions of items! > Andrew. Thanks, Uwe
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
Here's a question for you: could you tolerate a closeable mapped ByteBuffer which was somewhat slower to access? It is hard to make a secure closeable mapped ByteBuffer: the question is how small the overhead of closeability can be made. But if the answer is "We can't tolerate *any* additional overhead, no matter how small" then there is no motivation to do any experiments. Andrew.
RE: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
(reposting with correct sender address) Hi, > Note: some popular open source libraries that hack into the internal > private cleaner field of direct buffers will have to have their code > updated, if they wish to continue to do this. I followed parts of this issue, but as representative from Apache Lucene, which really really needs to un-mmap MappedByteBuffers (otherwise Lucene gets unuseable with JDK 9), I have no idea what the fix for us should be? What is the replacement for the following code (see marked BufferCleaner impl, which is our abstraction): https://goo.gl/DGYZZj The main reason why sun.misc.Cleaner was on the critical API list is NOT the case that somebody wanted to implement their own cleaner as replacement for finalization. The main reason (and this is *not* solved by the new APIs in java.lang.ref.Cleaner) is to *force* unmapping of direct and mmapped byte buffers. The unmapping of mmapped buffers is really needed and must be in Java 9, otherwise projects like Lucene will not work with Java 9, or may be slow as hell (if they use non-mmapped I/O) or otherwise run out of disk space (because mmapped files use disk space as they are never ever freed by the GC - see below). On Windows, in addition, files that are mmapped cannot be closed. These problem are the worst desaster that can happen to Lucene - a desaster like the big Hotspot issues back in 2011. With this patch the above code part would no longer work, because you can still make the getCleaner method available with setAccessible in Java 9 Jigsaw, but invoking the clean() method on the cleaner will horribly fail, because the cleaner is part of an internal package. So you have 2 possibilities: - Re-add sun.misc.Cleaner with a limited API (only the public clean() method) and let DirectBuffer return an instance of this sun.misc.Cleaner public API, so it is accessible without breaking encapsulation by adding extra exports - Or fix the full issue: Add "official" support to unmmap mmapped files. Really! You have to do this, otherwise it's a desaster (see below for the "why"). Mark Reinhold: I will as every year contact you on the FOSDEM dinner about sun.misc.Cleaner and unmapping DirectBuffers :-) Be sure to have a heated discussion! FYI: The reason why unmapping mmapped buffers is really needed is the following: MappedByteBuffer/DirectBuffers are very small objects (GC wise). Those are created by Lucene on mapping huuge files (several tens or hundreds of Gigabytes for some large indexes). Those objects live for longer time (up to several hours). But from time to time, Lucene releases the buffers and maps new ones (when updates occur on disk). Because those small - only a few bytes for the Garbage Collector - buffer instances lived so long, there is no reason to free them asap by GC. In fact, they are never freed if your application runs for several days (as Lucene/SOlr/Elasticsearch servers are living). So the small 20 byte objects hang forever on gigabytes of data! No way to realease that! So Lucene/Solr/Elasticserach's misuse of sun.misc.Cleaner is just to force the DirectBuffers to free the huge amounts of resources they hold. The new API of java.lang.ref.Cleaner does not help at all with this. You are just hiding the only possible workaround! Sorry for the bad news, but there needs to be a decision! We are great supporters of Jigsaw, but if we have to disable Jigsaw encapsulation, just to get access to jdk.internal.**.Cleaner, you are destroying the additional encapsulation for us! Please add a new issue to either revert to have sun.misc.Cleaner available just to forcefully free mmapped files, or add an official API to allow unmapping (including Hotspot changes to prevent SIGSEGV/SIGBUS). Uwe - Uwe Schindler uschind...@apache.org ASF Member, Apache Lucene PMC / Committer Bremen, Germany http://lucene.apache.org/ > -Original Message- > From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On > Behalf Of Chris Hegarty > Sent: Saturday, January 23, 2016 5:17 PM > To: core-libs-dev > Subject: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref > > sun.misc.Cleaner was previously listed as a critical internal API in JEP > 260 [1], but on further investigation it has been moved to an open issue, > for the following reasons: > 1) its primary use in the JDK is within NIO direct buffers to release > native memory. The base module cannot have a dependency on the > jdk.unsupported module so will need to be updated to use an > alternative cleaner, > 2) the usage of Cleaner outside the JDK, as determined by corpus > analysis, has largely been observed to hack into private fields of > the internal NIO direct buffer classes to explicitly release native > memory. > > As stated in 1), the type of the cleaner used by NIO dir
Re: RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
On 23/01/2016 16:16, Chris Hegarty wrote: : Webrev: http://cr.openjdk.java.net/~chegar/8148117/ This has to move to your patch looks okay. You might need to update the TEST.groups to ensure that the existOnThrow tests will be run by jdk_core. -Alan.
RFR [9] 8148117: Move sun.misc.Cleaner to jdk.internal.ref
sun.misc.Cleaner was previously listed as a critical internal API in JEP 260 [1], but on further investigation it has been moved to an open issue, for the following reasons: 1) its primary use in the JDK is within NIO direct buffers to release native memory. The base module cannot have a dependency on the jdk.unsupported module so will need to be updated to use an alternative cleaner, 2) the usage of Cleaner outside the JDK, as determined by corpus analysis, has largely been observed to hack into private fields of the internal NIO direct buffer classes to explicitly release native memory. As stated in 1), the type of the cleaner used by NIO direct buffers will have to change. Given this, and the fact that JDK 9 has a new general purposed cleaner API, java.lang.ref.Cleaner [2] the value of keep sun.misc.Cleaner is questionable. This issue proposes to simply move Cleaner into an internal non-exported package in the base module so that it can be used by the NIO direct buffers classes, and small number of other places. Webrev: http://cr.openjdk.java.net/~chegar/8148117/ If, at some point in the future, it is determined that sun.misc.Cleaner is in fact a critical internal API ( with static usage ), then it can be reinstated as a subtype of jdk.internal.ref.Cleaner, providing the same public API. Note: some popular open source libraries that hack into the internal private cleaner field of direct buffers will have to have their code updated, if they wish to continue to do this. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8132928 [1] https://bugs.openjdk.java.net/browse/JDK-8138696
