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

Wed Dec 5 10:37:27 UTC 2018

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