[9] RFR (M): 8139595: MethodHandles::remove_dependent_nmethod is not MT safe

[Vladimir Ivanov]

Hey! Any Reviews, please? :-)

Let me know if the overall approach looks sane to you.

Best regards,
Vladimir Ivanov

On 11/3/15 9:35 PM, Vladimir Ivanov wrote:
> http://cr.openjdk.java.net/~vlivanov/8139595/webrev.00
> https://bugs.openjdk.java.net/browse/JDK-8139595
>
> (Thanks to Stefan Karlsson who found the root cause.)
>
> Operations on nmethod dependency contexts (linked lists of
> nmethodBucket*) are either guarded by CodeCache lock or happen during
> safepoints.
>
> Recent GC enhancements introduced parallel code cache processing during
> GC, so parallel GC threads started to concurrently iterate over nmethod
> dependency lists, and it became not safe to remove stale elements
> (count() == 0) anymore.
>
> The initial fix was to delay stale element purging and do a serial pass,
> but it works only for contexts rooted at InstanceKlasses, GC does a
> serial pass over InstanceKlasses afterwards and deletes all stale
> entries. But it doesn't work for call_site_target dependencies which are
> part of contexts rooted at CallSiteContext Java objects.
>
> The problem manifests itself as a crash during dependency list traversal.
>
> The fix I propose is to avoid purging of CallSiteContext contexts at all
> during GC, but remove stale entries during dependency list updates which
> happen under CodeCache lock (see safe_to_purge() check in
> methodHandles.cpp). It happens outside of safepoints and it guarantees
> exclusive access to the list. It is performed during
> j.l.i.CallSite.target updates, CallSite.target inlining, j.l.Class
> unloading, etc. Such approach allows to avoid unbounded memory
> consumption growth, only some delay (till next dependency list update)
> in memory deallocation.
>
> To reduce amount of work at runtime I introduced a dependency list flag,
> which signals there are stale entries. It allows to skip purging if
> there are no stale entries in the list.
>
> As a cleanup I decided to refactor nmethod dependency management code.
>
> I came up with a DependencyContext to encapsulate the logic and used
> CPSlot idea (thanks for the pointer, John) to pack pointer + bool into a
> pointer.
>
> Here's the excerpt from the DependencyContext description:
> "Utility class to manipulate nmethod dependency context. The context
> consists of nmethodBucket* (a head of a linked list) and a boolean flag
> (does the list contains stale entries). The structure is encoded as an
> intptr_t: lower bit is used for the flag. It is possible since
> nmethodBucket* is aligned - the structure is malloc'ed in C heap.
> Dependency context can be attached either to an InstanceKlass
> (_dep_context field) or CallSiteContext oop for call_site_target
> dependencies (see javaClasses.hpp). DependencyContext class operates on
> some location which holds a intptr_t value."
>
> DependencyContext operates either on InstanceKlass::_dep_context or
> j.l.i.MHN$CallSiteContext::vmdependencies fields.
>
> It allows to remove bool field from InstanceKlass and avoid header
> allocation for CallSiteContext.
>
> Also, I experimented with non-packed version: have 2 fields as is and
> embed DependencyContext into InstanceKlass, but allocate it for
> CallSiteContext. The downsides were: (1) InstanceKlass footprint
> increase in some cases; (2) additional code for CDS case, since
> _dep_context is not shareable; (3) slight native memory footprint
> increase when there are many CallSite instances.
>
> So, I decided to proceed with packed version.
>
> Testing: hotspot/test/compiler/jsr292/CallSiteDepContextTest.java,
> -XX:+ExecuteInternalVMTests, JPRT
>
> Thanks!
>
> Best regards,
> Vladimir Ivanov