[9] RFR(S): 8029443: 'assert(klass->is_loader_alive(_is_alive)) failed: must be alive' during VM_CollectForMetadataAllocation

Wed Jul 16 08:34:08 UTC 2014

Hi Tobias,

On Wednesday 16 July 2014 10.24.16 Tobias Hartmann wrote:
> Hi Mikael,
> 
> thanks for the review. Please see comments inline.
> 
> On 15.07.2014 13:36, Mikael Gerdin wrote:
> > Tobias,
> > 
> > On Tuesday 15 July 2014 09.48.08 Tobias Hartmann wrote:
> >> Hi Vladimir,
> >> 
> >>> Impressive work, Tobias!
> >> 
> >> Thanks! Took me a while to figure out what's happening.
> >> 
> >>> So before the permgen removal embedded method* were oops and they were
> >>> processed in relocInfo::oop_type loop.
> >> 
> >> Okay, good to know. That explains why the terms oops and metadata are
> >> used interchangeably at some points in the code.
> > 
> > Yep, there are a lot of leftover references to metadata as oops,
> > especially in some compiler/runtime parts such as MDOs and CompiledICs.
> > 
> >>> May be instead of specializing opt_virtual_call_type and
> >>> static_call_type call site you can simple add a loop for
> >>> relocInfo::metadata_type (similar to oop_type loop)?
> >> 
> >> The problem with iterating over relocInfo::metadata_type is that we
> >> don't know to which stub, i.e., to which IC the Method* pointer belongs.
> >> Since we don't want to unload the entire method but only clear the
> >> corresponding IC, we need this information.
> > 
> > I'm wondering, is there some way to figure out the IC for the Method*?
> > 
> > In CompiledStaticCall::emit_to_interp_stub a static_stub_Relocation is
> > created and from the looks of it it points to the call site through some
> > setting of a "mark".
> > 
> > The metadata relocation is emitted just after the static_stub_Relocation,
> > so one approach (untested) could be to have a case for
> > static_stub_Relocations, create a CompiledIC.at(reloc->static_call()) and
> > check if it's a call to interpreted. If it is the advance the
> > relocIterator to the next position and check that metadata for liveness.
> 
> The relocation entries for this particular case are [1]. Looking at the
> static_stub_Relocation (0xffffffff6ea49cc4) we don't know if the stub
> belongs to an optimized IC or a compiled static call. We would either
> have to create both CompiledIC.at(..) and compiledStaticCall_at(..) or
> check the relocation entry for the call (0xffffffff6ea49bc0) requiring
> another iteration. Only then we are able to look at the
> metadata_relocation at the next position.

That's a good point.

> 
> Since we already have case statements for opt_virtual_call_type and
> static_call_type (at least in nmethod::do_unloading_parallel(..)) I
> would prefer to infer the Method* from the IC or compiled static call.
> 
> I adapted the implementation according to Coleen's feedback:
> 
> http://cr.openjdk.java.net/~thartmann/8029443/webrev.01/
> 
> What do you think?

I think your suggested change is fine.

Did you test it with -XX:+UseG1GC to exercise the do_unloading_parallel path 
as well?

/Mikael

> 
> Thanks,
> Tobias
> 
> 
> [1] Relocation entries:
> 
> [...]
> 154031          @0xffffffff6ea49b3a: 3005
> 154032 relocInfo at 0xffffffff6ea49b3a [type=3(opt_virtual_call)
> addr=0xffffffff6ea49bc0 offset=20]
> [...]
> 154047          @0xffffffff6ea49b52: f801ffe8500a
> 154048 relocInfo at 0xffffffff6ea49b56 [type=5(static_stub)
> addr=0xffffffff6ea49cc4 offset=40 data=-24] |
> [static_call=0xffffffff6ea49bc0]
> 154049          @0xffffffff6ea49b58: f003c000
> 154050 relocInfo at 0xffffffff6ea49b5a [type=12(metadata)
> addr=0xffffffff6ea49cc4 offset=0 data=3] |
> [metadata_addr=0xffffffff6ea49d68 *=0xffffffff6ae20960
> offset=0]metadata_value=0xffffffff6ae20960: {method}
> {0xffffffff6ae20968} 'newInstance'
> '([Ljava/lang/Object;)Ljava/lang/Object;' in
> 'sun/reflect/GeneratedConstructorAccessor3'
> 154051          @0xffffffff6ea49b5c: f003c007
> 154052 relocInfo at 0xffffffff6ea49b5e [type=12(metadata)
> addr=0xffffffff6ea49ce0 offset=28 data=3] |
> [metadata_addr=0xffffffff6ea49d68 *=0xffffffff6ae20960
> offset=0]metadata_value=0xffffffff6ae20960: {method}
> {0xffffffff6ae20968} 'newInstance'
> '([Ljava/lang/Object;)Ljava/lang/Object;' in
> 'sun/reflect/GeneratedConstructorAccessor3'
> 
> 154053          @0xffffffff6ea49b60:
> > /Mikael
> > 
> >> Thanks,
> >> Tobias
> >> 
> >>> Thanks,
> >>> Vladimir
> >>> 
> >>> On 7/14/14 4:56 AM, Tobias Hartmann wrote:
> >>>> Hi,
> >>>> 
> >>>> please review the following patch for JDK-8029443.
> >>>> 
> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8029443
> >>>> Webrev: http://cr.openjdk.java.net/~thartmann/8029443/webrev.00/
> >>>> 
> >>>> *Problem*
> >>>> After the tracing/marking phase of GC, nmethod::do_unloading(..) checks
> >>>> if a nmethod can be unloaded because it contains dead oops. If class
> >>>> unloading occurred we additionally clear all ICs where the cached
> >>>> metadata refers to an unloaded klass or method. If the nmethod is not
> >>>> unloaded, nmethod::verify_metadata_loaders(..) finally checks if all
> >>>> metadata is alive. The assert in CheckClass::check_class fails because
> >>>> the nmethod contains Method* metadata corresponding to a dead Klass.
> >>>> The Method* belongs to a to-interpreter stub [1] of an optimized
> >>>> compiled IC. Normally we clear those stubs prior to verification to
> >>>> avoid dangling references to Method* [2], but only if the stub is not
> >>>> in
> >>>> use, i.e. if the IC is not in to-interpreted mode. In this case the
> >>>> to-interpreter stub may be executed and hand a stale Method* to the
> >>>> interpreter.
> >>>> 
> >>>> *Solution
> >>>> *The implementation of nmethod::do_unloading(..) is changed to clean
> >>>> compiled ICs and compiled static calls if they call into a
> >>>> to-interpreter stub that references dead Method* metadata.
> >>>> 
> >>>> The patch was affected by the G1 class unloading changes (JDK-8048248)
> >>>> because the method nmethod::do_unloading_parallel(..) was added. I
> >>>> adapted the implementation as well.
> >>>> *
> >>>> Testing
> >>>> *Failing test (runThese)
> >>>> JPRT
> >>>> 
> >>>> Thanks,
> >>>> Tobias
> >>>> 
> >>>> [1] see CompiledStaticCall::emit_to_interp_stub(..)
> >>>> [2] see nmethod::verify_metadata_loaders(..),
> >>>> static_stub_reloc()->clear_inline_cache() clears the stub