RFR (M): 8079205: CallSite dependency tracking is broken after sun.misc.Cleaner became automatically cleared
paul.sandoz at oracle.com
Wed May 13 11:24:07 UTC 2015
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.
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?
1182 static oop context(oop site);
1183 static void set_context(oop site, oop context);
Is set_context required?
1876 // recording of dependecies. Returns true if the bucket is ready for reclamation.
On May 13, 2015, at 11:27 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
> I finished experimenting with the idea inspired by private discussion with Kim Barrett:
> 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
> Best regards,
> Vladimir Ivanov
> On 5/12/15 1:56 PM, Vladimir Ivanov wrote:
>>> 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
>>>> 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
>> 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 ,
>>> 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
>>>>  http://cr.openjdk.java.net/~vlivanov/8079205/webrev.00
More information about the core-libs-dev