Hi Coleen,

This looks great. Thanks for sorting this out!

/Erik

On 12/2/19 3:43 PM, coleen.phillim...@oracle.com wrote:


On 11/26/19 7:03 PM, David Holmes wrote:
(adding runtime as well)

Hi Coleen,

On 27/11/2019 12:22 am, coleen.phillim...@oracle.com wrote:
Summary: Add local deferred event list to thread to post events outside CodeCache_lock.

This patch builds on the patch for JDK-8173361.  With this patch, I made the JvmtiDeferredEventQueue an instance class (not AllStatic) and have one per thread.  The CodeBlob event that used to drop the CodeCache_lock and raced with the sweeper thread, adds the events it wants to post to its thread local list, and processes it outside the lock.  The list is walked in GC and by the sweeper to keep the nmethods from being unloaded and zombied, respectively.

Sorry I don't understand why we would want/need a deferred event queue for every JavaThread? Isn't this only relevant for non-JavaThreads that need to have the ServiceThread process the deferred event?

I thought I'd written this in the bug but I had only discussed this with Erik.  I've added a comment to the bug to explain why I added the per-JavaThread queue.  In order to process these events after the CodeCache_lock is dropped, I have to queue them somewhere safe. The ServiceThread queue is safe, *but* the ServiceThread can't keep up with the events, especially from this test case.  So the test case gets a native OOM.

So I've added the safe queue as a field to each JavaThread because multiple JavaThreads could be posting these events at the same time, and there didn't seem to be a better safe place to cache them, without adding another layer of queuing code.

I did write comments to this effect here:

http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev/src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp.udiff.html

Thanks,
Coleen


David

Also, the jmethod_id field in nmethod was only used as a boolean so don't create a jmethod_id until needed for post_compiled_method_unload.

Ran hs tier1-8 on linux-x64-debug and the stress test that crashed in the original bug report.

open webrev at http://cr.openjdk.java.net/~coleenp/2019/8212160.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8212160

Thanks,
Coleen


Reply via email to