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

[Kim Barrett]

On Wed, 30 Nov 2022 20:15:47 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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.
>
> @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.

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

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