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

[dean.long at oracle.com]

On 11/30/18 3:19 AM, Erik Österlund wrote:
> 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.
>

Is it correct to say that the loop was there before (in new_ic_stub), 
but now you've moved it into the callers?

dl

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