RFR: 8316959: Improve InlineCacheBuffer pending queue management [v3]

[Thomas Schatzl]

On Mon, 9 Oct 2023 22:50:06 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   dlong review
>
> src/hotspot/share/code/icBuffer.cpp line 272:
> 
>> 270:   CompiledICHolder* old = Atomic::load(&_pending_released);
>> 271:   for (;;) {
>> 272:     icholder->set_next(old);
> 
> Could we do this only after the swap is successful?

I think it could be done since nobody is actually looking at the list until a safepoint where it should be fixed up. What would be the benefit of doing this?

The current implementation is imo a fairly idiomatic/unsurprising way adding nodes to a linked list lock-free (in gc code, I did not look for examples in compiler code). I would probably expect any significiant deviation to be commented which seems unnecessary and just asking for the question "why is it done this way?".

The only thing I could think of would be that it would allow one to put the `next == nullptr` assert later, but that does not seem to be worth doing - the `CompiledICHolder`s are not shared, i.e. that multiple threads may add the same item (the previous locking code would have failed in this situation already). Executing the assert later will imo not significantly increase the possibility of failure (assuming that the far majority of additions are uncontended).

If you really prefer I can move the `set_next` call.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16059#discussion_r1352340994