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

[Dean Long]

On Tue, 10 Oct 2023 12:11:14 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

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

It would avoid a unneeded set_next every time we loop on contention, but it's probably not worth changing just to optimize the contended case, and like you said the code is less surprising this way, so just leave it.

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

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