RFR: 8331911: Reconsider locking for recently disarmed nmethods [v3]

[Neethu Prasad]

On Fri, 7 Jun 2024 20:14:01 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>>> Having said that, perhaps we should file a separate issue to remove that check, since it seems to fix an actual bug, while I guess this was meant more as an optimization. What do you think?
>> 
>> FTR, I don't mind executing cross-modify-fence unconditionally. I do mind going into deopts too often. I do also think that we want to stay on performance-positive side for at least an easy variant of fix, and do potentially regressing things separately. The initial motivation for this work was to resolve an issue in a service workload that runs many threads with similar stacks, and get something that we are sure about for a prompt backport.
>> 
>> To that end, we can continue working out the final shape of the patch here, while we mitigate our current service problems with picking up a limited version of this patch with [JDK-8333716](https://bugs.openjdk.org/browse/JDK-8333716) -- it resolves only Shenandoah parts of it, though. Or, we can integrate this patch in its current form, resolving the issue on both Shenandoah and ZGC paths, and work out the check removal as the follow up of [JDK-8310239](https://bugs.openjdk.org/browse/JDK-8310239).
>> 
>> I think the latter alternative is more pragmatic.
>
>> > Having said that, perhaps we should file a separate issue to remove that check, since it seems to fix an actual bug, while I guess this was meant more as an optimization. What do you think?
>> 
>> FTR, I don't mind executing cross-modify-fence unconditionally. I do mind going into deopts too often. I do also think that we want to stay on performance-positive side for at least an easy variant of fix, and do potentially regressing things separately. The initial motivation for this work was to resolve an issue in a service workload that runs many threads with similar stacks, and get something that we are sure about for a prompt backport.
> 
> Fair enough. For what it's worth, aside for the deopt stressing option with arbitrary frequency we can update, we will not deopt more. We just perform an extra cross modifying fence when racingly entering an nmethod concurrently being disarmed. Not performing it might be slightly faster, but is a bug. But I see your point.
> 
>> To that end, we can continue working out the final shape of the patch here, while we mitigate our current service problems with picking up a limited version of this patch with [JDK-8333716](https://bugs.openjdk.org/browse/JDK-8333716) -- it resolves only Shenandoah parts of it, though. Or, we can integrate this patch in its current form, resolving the issue on both Shenandoah and ZGC paths, and work out the check removal as the follow up of [JDK-8310239](https://bugs.openjdk.org/browse/JDK-8310239).
>> 
>> I think the latter alternative is more pragmatic.
> 
> I'm okay with approving this patch, and we fix the actual bug separately. Sounds good? Then this is a refactoring and optimization, without the bug fix.

@fisk I just merged the latest changes. Do I need approval on the merge commit or can I integrate?

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

PR Comment: https://git.openjdk.org/jdk/pull/19285#issuecomment-2186951944