RFR: 8305252: make_method_handle_intrinsic may call java code under a lock [v3]

[Coleen Phillimore]

On Wed, 19 Apr 2023 19:33:43 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This patch releases the InvokeMethodTable_lock while creating a method handle intrinsic.  If there's a race, it frees a Method created by racing thread.  The logic is simple but uses the deallocate_list infrastructure that's mostly used for redefinition making it less rare.  With Dacapo2009, this adds about 20 Methods + constant pools to the list.  Also the method has to call nmethod->flush which is assumed to be something only GC calls.
>> 
>> Tested with tier1-4.
>
> Coleen Phillimore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into invoketable
>  - Update method deallocate_contents comments.
>  - 8305252: make_method_handle_intrinsic may call java code under a lock

Talking with @fisk makes me think the nmethod->flush is unsafe in the long term.  The nmethod->flush takes out the CodeCache_lock, but deoptimization uses a RelaxedCompiledMethodIterator to iterate over nmethods.  It only takes out the CodeCache_lock when calling next().  It assumes that the nmethod it returns won't be concurrently destroyed by GC, but this code also destroys an nmethod.

This code is only called during a dedicated safepoint so is safe from deoptimization for now.

But ClassLoaderDataGraph::purge can be called concurrently.  CLDG::purge doesn't call Method::deallocate_contents because the metadata is released in bulk, but if this intrinsic Method is on the deallocate list we call free_deallocate_list_C_heap_structures() which doesn't flush the nmethod for this Method.  So an nmethod in the code cache would point to a deleted Method.  But it can't call flush() because this isn't in a safepoint and violate the code cache walking assumptions.

The only ClassLoaderData that have these Methods on the deallocate_lists are ones that are never unloaded so this is safe for now.

The "for now" safety is why I'm closing this PR and going with a different approach.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13308#issuecomment-1516750155