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

[Erik Österlund]

Hi Rickard,

Thanks for the review!

/Erik

On 2018-12-05 11:37, Rickard Bäckman wrote:
> Glad to hear that debugging code actually paid off!
> Looks good!
> 
> /R
> 
> On 12/05, Erik Österlund wrote:
>> 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
>>>>>>>
>>>>>
> /R
>