RFR: 8214338: Move IC stub refilling out of IC cache transitions

[Erik Österlund]

Hi Dean,

Thanks for reviewing this!

On 2018-11-27 23:55, dean.long at oracle.com wrote:
> There's a typo in the new guarantee message: "tranisition".

Fixed in-place.

> Regarding the infinite loops, can we detect if progress isn't being made 
> and assert/panic, rather than hanging?

I'm afraid not. It's a similar analogy to the problem of progress 
guarantees of a lock-free algorithm. They have a global progress 
guarantee for the system, but no local guarantee of progress for each 
operation, that can in theory be starved indefinitely due to other 
operations winning every race. The situation is similar here, at least 
in theory, that the thread refilling the IC stubs didn't get a single 
stub back, because another thread started depleted the IC stubs again. 
Naturally, if this was ever to happen, we would get time outs in our 
tests though, to indicate that there is a problem.

Thanks,
/Erik

> dl
> 
> On 11/27/18 1:29 PM, Erik Österlund wrote:
>> Hi Dean,
>>
>> Thank you for reviewing this.
>>
>> Full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8214338/webrev.01/
>>
>> Incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8214338/webrev.00_01/
>>
>> On 2018-11-27 19:07, dean.long at oracle.com wrote:
>>> Hi Erik.  Can you explain why you removed the pre-allocated "next 
>>> stub"?  I guess it was no longer necessary?  If so, then you should 
>>> update the "always has sentinel" comment in is_empty.
>>
>> Updated. Indeed, the sentinel no longer serves a purpose so I removed 
>> it. Found some more traces of the sentinel that I removed in the last 
>> webrev.
>>
>>> In your guarantee messages, some use "should not fail" and some use 
>>> "should never fail".  It's not a big deal but maybe they should be 
>>> the same.
>>
>> Sure. Updated.
>>
>>> You introduced a couple of infinite loops.  Are we guaranteed to exit 
>>> these loops eventually?  Is there an upper bound on how many 
>>> iterations are needed?
>>
>> In my latest webrev I removed some unnecessary set_to_clean() where IC 
>> caches are already is_clean(). With that in place, there should be 
>> global progress guarantees and a single IC stub in the buffer should 
>> be all you really "need". Although, you might want more. In fact, I'm 
>> questioning if the 10K size buffer is big enough today, but I'll leave 
>> that for another day.
>>
>> Thanks,
>> /Erik
>>
>>> The rest looks good.
>>>
>>> dl
>>>
>>> On 11/27/18 5:00 AM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> IC stub refilling currently requires a safepoint operation. When 
>>>> done right at the point where an CompiledIC is about to get patched 
>>>> to a transitional state using an IC stub, locks may already be held, 
>>>> causing a bunch of locking issues - especially for concurrent class 
>>>> unloading. Therefore, the IC stub refilling ought to be moved out so 
>>>> that IC cache cleaning and transitioning may be done without any 
>>>> safepoints, and the locks in the path ought to not perform safepoint 
>>>> checking.
>>>>
>>>> This is implemented by allowing IC transitions to fail when they 
>>>> require IC stubs, and we run out of them. This propages back to a 
>>>> higher level where IC stubs are refilled after having released the 
>>>> IC locker.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8214338/webrev.00/
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8214338
>>>>
>>>> Thanks,
>>>> /Erik
>>>
>