Hi Vladimir, I am not an export in the HS area but the code mostly made sense to me. I also like Peter's suggestion of Context implementing Runnable.
Some minor comments. CallSite.java: 145 private final long dependencies = 0; // Used by JVM to store JVM_nmethodBucket* It's a little odd to see this field be final, but i think i understand the motivation as it's essentially acting as a memory address pointing to the head of the nbucket list, so in Java code you don't want to assign it to anything other than 0. Is VM access, via java_lang_invoke_CallSite_Context, affected by treating final fields as really final in the j.l.i package? javaClasses.hpp: 1182 static oop context(oop site); 1183 static void set_context(oop site, oop context); Is set_context required? instanceKlass.cpp: 1876 // recording of dependecies. Returns true if the bucket is ready for reclamation. typo "dependecies" Paul. On May 13, 2015, at 11:27 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> wrote: > I finished experimenting with the idea inspired by private discussion with > Kim Barrett: > > http://cr.openjdk.java.net/~vlivanov/8079205/webrev.02/ > > The idea is to use CallSite instance as a context for dependency tracking, > instead of the Class CallSite is bound to. It requires extension of nmethod > dependencies to be used separately from InstanceKlass. Though it slightly > complicates nmethod dependency management (special cases for > call_site_target_value are added), it completely eliminates the need for > context state transitions. > > Every CallSite instance has its dedicated context, represented as > CallSite.Context which stores an address of the dependency list head > (nmethodBucket*). Cleanup action (Cleaner) is attached to CallSite instance > and frees all allocated nmethodBucket entries when CallSite becomes > unreachable. Though CallSite instance can freely go away, the Cleaner keeps > corresponding CallSite.Context from reclamation (all Cleaners are registered > in static list in Cleaner class). > > I like how it finally shaped out. It became much easier to reason about > CallSite context. Dependency tracking extension to non-InstanceKlass > contextes looks quite natural and can be reused. > > Testing: extended regression test, jprt, nashorn > > Thanks! > > Best regards, > Vladimir Ivanov > > On 5/12/15 1:56 PM, Vladimir Ivanov wrote: >> Peter, >> >>>> http://cr.openjdk.java.net/~vlivanov/8079205/webrev.01 >>>> https://bugs.openjdk.java.net/browse/JDK-8079205 >>> >>> Your Finalizator touches are good. Supplier interface is not needed as >>> there is a public Reference superclass that can be used for return type >>> of JavaLangRefAccess.createFinalizator(). You can remove the import for >>> Supplier in Reference now. >> Thanks for spotting that. Will do. >> >>>> Recent change in sun.misc.Cleaner behavior broke CallSite context >>>> cleanup. >>>> >>>> CallSite references context class through a Cleaner to avoid its >>>> unnecessary retention. >>>> >>>> The problem is the following: to do a cleanup (invalidate all affected >>>> nmethods) VM needs a pointer to a context class. Until Cleaner is >>>> cleared (and it was a manual action, since Cleaner extends >>>> PhantomReference isn't automatically cleared according to the docs), >>>> VM can extract it from CallSite.context.referent field. >>> >>> If PhantomReference.referent wasn't cleared by VM when PhantomReference >>> was equeued, could it happen that the referent pointer was still != null >>> and the referent object's heap memory was already reclaimed by GC? Is >>> that what JDK-8071931 is about? (I cant't see the bug - it's internal). >>> In that respect the Cleaner based solution was broken from the start, as >>> you did dereference the referent after Cleaner was enqueued. You could >>> get a != null pointer which was pointing to reclaimed heap. In Java this >>> dereference is prevented by PhantomReference.get() always returning null >>> (well, nothing prevents one to read the referent field with reflection >>> though). >> No, the object isn't reclaimed until corresponding reference is != null. >> When Cleaner wasn't automatically cleared, it was guaranteed that the >> referent is alive when cleanup action happens. That is the assumption >> CallSite dependency tracking is based on. >> >> It's not the case anymore when Cleaner is automatically cleared. Cleanup >> action can happen when context Class is already GCed. So, I can access >> neither the context mirror class nor VM-internal InstanceKlass. >> >>> FinalReference(s) are different in that their referent is not reclaimed >>> while it is still reachable through FinalReference, which means that >>> finalizable objects must undergo at least two GC cycles, with processing >>> in the reference handler and finalizer threads inbetween the cycles, to >>> be reclaimed. PhantomReference(s), on the other hand, can be enqueued >>> and their referents reclaimed in the same GC cycle, can't they? >> Since PhantomReference isn't automatically cleared, GC can't reclaim its >> referent until the reference is manually cleared or PhantomReference >> becomes unreachable as a result of cleanup action. >> So, it's impossible to reclaim it in the same GC cycle (unless it is a >> concurrent GC cycle?). >> >>>> I experimented with moving cleanup logic into VM [1], >>> >>> What's the downside of that approach? I mean, why is GC-assisted >>> approach better? Simpler? >> IMO the more code on JDK side the better. More safety guarantees are >> provided in Java code comparing to native/VM code. >> >> Also, JVM-based approach suffers from the fact it doesn't get prompt >> notification when context Class can be GCed. Since it should work with >> autocleared references, there's no need in a cleanup action anymore. >> By the time cleanup happens, referent can be already GCed. >> >> That's why I switched from Cleaner to WeakReference. When >> CallSite.context is cleared ("stale" context), VM has to scan all >> nmethods in the code cache to find all affected nmethods. >> >> BTW I had a private discussion with Kim Barrett who suggested an >> alternative approach which doesn't require full code cache scan. I plan >> to do some prototyping to understand its feasibility, since it requires >> non-InstanceKlass-based nmethod dependency tracking machinery. >> >> Best regards, >> Vladimir Ivanov >> >>>> but Peter Levart came up with a clever idea and implemented >>>> FinalReference-based cleaner-like Finalizator. Classes don't have >>>> finalizers, but Finalizator allows to attach a finalization action to >>>> them. And it is guaranteed that the referent is alive when >>>> finalization happens. >>>> >>>> Also, Peter spotted another problem with Cleaner-based implementation. >>>> Cleaner cleanup action is strongly referenced, since it is registered >>>> in Cleaner class. CallSite context cleanup action keeps a reference to >>>> CallSite class (it is passed to MHN.invalidateDependentNMethods). >>>> Users are free to extend CallSite and many do so. If a context class >>>> and a call site class are loaded by a custom class loader, such loader >>>> will never be unloaded, causing a memory leak. >>>> >>>> Finalizator doesn't suffer from that, since the action is referenced >>>> only from Finalizator instance. The downside is that cleanup action >>>> can be missed if Finalizator becomes unreachable. It's not a problem >>>> for CallSite context, because a context is always referenced from some >>>> CallSite and if a CallSite becomes unreachable, there's no need to >>>> perform a cleanup. >>>> >>>> Testing: jdk/test/java/lang/invoke, hotspot/test/compiler/jsr292 >>>> >>>> Contributed-by: plevart, vlivanov >>>> >>>> Best regards, >>>> Vladimir Ivanov >>>> >>>> PS: frankly speaking, I would move Finalizator from java.lang.ref to >>>> java.lang.invoke and call it Context, if there were a way to extend >>>> package-private FinalReference from another package :-) >>> >>> Modules may have an answer for that. FinalReference could be moved to a >>> package (java.lang.ref.internal or jdk.lang.ref) that is not exported to >>> the world and made public. >>> >>> On the other hand, I don't see a reason why FinalReference couldn't be >>> part of JDK public API. I know it's currently just a private mechanism >>> to implement finalization which has issues and many would like to see it >>> gone, but Java has learned to live with it, and, as you see, it can be a >>> solution to some problems too ;-) >>> >>> Regards, Peter >>> >>>> >>>> [1] http://cr.openjdk.java.net/~vlivanov/8079205/webrev.00 >>>
