[9] RFR (M): 8079205: CallSite dependency tracking is broken after sun.misc.Cleaner became automatically cleared

[Peter Levart]

Hi Vladimir,

This is nice. I don't know how space-savvy CallSite should be, but you 
can save one additional object - the Cleaner's thunk:

     static class Context implements Runnable {
         private final long dependencies = 0; // Used by JVM to store 
JVM_nmethodBucket*

         static Context make(CallSite cs) {
             Context newContext = new Context();
             // Cleaner is attached to CallSite instance and it clears 
native structures allocated for CallSite context.
             // Though the CallSite can become unreachable, its Context 
is retained by the Cleaner instance (which is
             // referenced from Cleaner class) until cleanup is performed.
             Cleaner.create(cs, newContext);
             return newContext;
         }

         @Override
         public void run() {
             MethodHandleNatives.clearCallSiteContext(this);
         }
     }

Since Context instance does not escape to client code, I think this is safe.

Regards, Peter


On 05/13/2015 11:27 AM, Vladimir Ivanov 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
>>>