RFR: 8297389: resexhausted003 fails with assert(!thread->owns_locks()) failed: must release all locks when leaving VM

[David Holmes]

On Wed, 23 Nov 2022 11:56:45 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> `Method::build_profiling_method_data` acquires the `MethodData_lock` when initializing `Method::_method_data` to prevent multiple allocations by different threads. The problem is that when metaspace allocation fails and `JvmtiExport::should_post_resource_exhausted()` is set, we assert during the `ThreadToNativeFromVM` transition in JVMTI code.
> 
> Since concurrent initialization is a rare event, I suggest to get rid of the lock and perform the initialization with a `cmpxchg`, similar to how method counters are initialized:
> https://github.com/openjdk/jdk/blob/f4b5065c37e86f4b2ca26da6ce678febe4a52950/src/hotspot/share/oops/method.cpp#L644-L646
> 
> Since [current code](https://github.com/openjdk/jdk/blob/f4b5065c37e86f4b2ca26da6ce678febe4a52950/src/hotspot/share/oops/method.inline.hpp#L41-L46) in `Method::set_method_data` uses a `Atomic::release_store`, I added a `OrderAccess::release()`.
> 
> Thanks,
> Tobias

If `MethodData::allocate` cannot be called whilst holding a lock then perhaps we can assert that in there?

I think there is a broader problem here that metaspace allocation can occur in numerous places and any failure could post the resource-exhausted event and potentially lead to problems like this.

This fix side-steps one problematic call-site, but going lock-free has its own concerns.

src/hotspot/share/oops/method.cpp line 594:

> 592: 
> 593:   ClassLoaderData* loader_data = method->method_holder()->class_loader_data();
> 594:   MethodData* method_data = MethodData::allocate(loader_data, method, THREAD);

So the downside of lock-free here is that we have to pre-allocate and then later free. How expensive is that? How likely are we to get multiple threads attempting this at the same time? We might trigger a resource-exhausted event unnecessarily due to the temporary use of metaspace.

src/hotspot/share/oops/method.cpp line 604:

> 602:   // total store order (TSO) the reference may become visible before
> 603:   // the initialization of data otherwise.
> 604:   OrderAccess::release();

A release here is not necessary as `Atomic::replace_if_null` will by default have `memory_order_conservative` which maintains the full bi-directional fence of the internal `cmpxchg`.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11316