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

[Erik Österlund]

Hi,

I talked to Rickard offline. He had some suggestions for improvement:

1) Exit early in CompiledMethod::clean_ic_if_metadata_is_dead if the ic 
is clean.
2) Make a template function for cleaning the reloc entries that do not 
expect IC cleaning to fail, rather than repeating the same guarantee 
message.
3) Split out the bodies of the two for loops that restart if we ran out 
of IC stubs, to separate functions so it becomes easier to read.
4) Add some extra debugging code. I added a counter that is incremented 
when we fail a transition due to IC stub depletion, and clear it when 
refilling IC stubs, and assert in the safepoint cleanup code that the 
counter is cleared. This catches mistakes where a transition failed, but 
nobody bothered to refill and try again. It actually caught a spot where 
an IC cache was cleaned without checking if successful in 
SharedRuntime::reresolve_call_site. So this was a great idea.

I also made it explicit when returning from set_to_megamorphic if it 
failed due to running out of IC stubs or vtable stubs. Only in the case 
of running out of IC stubs do we need to refill IC stubs.

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8214338/webrev.02/

Incremental:
http://cr.openjdk.java.net/~eosterlund/8214338/webrev.01_02/

Thanks,
/Erik

On 2018-12-03 16:39, Erik Österlund wrote:
> Any more takers? I need one more review. I have passed hs-tier1-6 twice 
> with this now.
> 
> /Erik
> 
> On 2018-11-30 12:19, 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.
>>
>> 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
>>>>>
>>>