RFR: 8256302: releasing oopStorage when deflating allows for faster deleting [v3]

[Thomas Stuefe]

On Sat, 3 Dec 2022 04:25:07 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> @coleenp
>>> The code prior to your change did chk_for_block_req, ie checked for a safepoint. I was asking above why it did this in this loop since the code just above does a handshake. That is, why would it be necessary?
>> 
>> `deflate_monitor_list()` will deflate up to `MonitorDeflationMax` ObjectMonitors.
>> The default for that value is `1000000`. So when we're processing the "delete_list"
>> we used to call `chk_for_block_req()` so that we didn't cause too much of a wait
>> for the next safepoint (or handshake). We did this to avoid impacting time-to-safepoint.
>> 
>>> This code is generally run in another thread.
>> 
>> Yes, in the MonitorDeflation thread. It's a JavaThread so the VM Thread will wait
>> for it for safepoints.
>> 
>> So @robehn's idea is to split the release apart from the free. We do the release
>> when we deflate (or when we throw away a newly allocated ObjectMonitor) and
>> we do the free while blocked. Even less impact on time-to-safepoint.
>> 
>>> Is the safepoint check the slow part or is the OopStorage release()? Or both? Do you have a test to point to that gets behind in freeing ObjectMonitors?
>> 
>> Doing the `chk_for_block_req()` call up to a million times is a bit costly. I don't
>> know how costly the `OopStorage release()` is, but we moved that for functional
>> reasons and not for performance reasons. Can't do `OopStorage release()` while
>> blocked. Doing a `delete/os::free()` can be costly depending on the C-library.
>> In any case, pushing work from the JavaThread being in VM mode to the JavaThread
>> being in blocked mode is a "good thing".
>> 
>>> Doing a os::free which may call NMT while the thread is in the blocked state is probably okay, but I think you should check this out. There may be a dcmd that runs at safepoint that does NMT things.
>> 
>> That's an interesting question. I can't imagine that this is the first call to os::free() that
>> can run in parallel with a safepoint. Normally, I would ask Zhengyu, but he's not working
>> on this stuff anymore.
>> 
>> For what it's worth, I've run these changes thru several Tier[1-8] test runs without any
>> signs of issues...
>
> Just to make sure I'm understanding things correctly - it's not
> OopStorage::release that has thread state requirements, it's
> WeakHandle::release. The WeakHandle operation needs to ensure the pointee has
> been nulled out (before calling the OopStorage operation), invoking GC
> barriers.
> 
> If the claim is that OopStorage::release has thread state requirements, I'd
> like more info, since that function is intentionally trying to be low on
> requirements.

> > Doing a os::free which may call NMT while the thread is in the blocked state is probably okay, but I think you should check this out. There may be a dcmd that runs at safepoint that does NMT things.
> 
> That's an interesting question. I can't imagine that this is the first call to os::free() that can run in parallel with a safepoint. Normally, I would ask Zhengyu, but he's not working on this stuff anymore.

That is fine. In summary mode, NMT record_free will just decrease atomic counters. In detail mode, it will look up an entry from the malloc site table and decrease its atomic counters. Malloc site table is lockless. There is nothing that should block, or require safepoint-less-ness.

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

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