RFR 8029178: Parallel class loading test anonymous-simple gets SIGSEGV in Metaspace:, :contains

[Mikael Gerdin]

On Friday 03 January 2014 07.57.40 Coleen Phillmore wrote:
> Mikael,
> 
> Thanks for reviewing the code.  See below.
> 
> On 1/3/2014 4:31 AM, Mikael Gerdin wrote:
> > Coleen,
> > 
> > On Thursday 02 January 2014 12.25.25 Coleen Phillmore wrote:
> > > Summary: Metaspace::contains cannot look at purged metaspaces while CMS
> > > 
> > > concurrently deallocates them.
> > > 
> > > 
> > > 
> > > Removed 2 calls to is_metaspace_object where the object may be in a
> > > 
> > > deallocated metaspace. Removed walking virtual space lists for
> > > 
> > > determining contains because the virtual space list can change
> > > 
> > > concurrently with the walk. CLDG::contains is slower but no slowdowns
> > > 
> > > with testing were observed.
> > > 
> > > 
> > > 
> > > Tested by SQE testbase tests, jtreg tests. Functional testing by
> > > 
> > > parallel class loading tests and nsk/coverage/arguments/arguments008
> > > 
> > > (ie. calls Method::is_valid_method)
> > > 
> > > 
> > > 
> > > open webrev at http://cr.openjdk.java.net/~coleenp/8029178/
> > 
> > --- old/src/share/vm/code/dependencies.cpp 2014-01-02
> > 12:04:06.325014397 -0500
> > 
> > +++ new/src/share/vm/code/dependencies.cpp	2014-01-02 
12:04:05.877014397
> > -0500 @@ -655,8 +655,6 @@
> > 
> >     } else {
> >     
> >       o = _deps->oop_recorder()->metadata_at(i);
> >     
> >     }
> > 
> > -  assert(o == NULL || o->is_metaspace_object(),
> > -         err_msg("Should be metadata " PTR_FORMAT, o));
> > 
> >     return o;
> >   
> >   }
> > 
> > Why did you remove only this call to is_metaspace_object and no other?
> > 
> > This and none of the other callers of is_metaspace_object seem to be
> > in any hot path so I agree with your assessment that this should not
> > be a performance issue.
> 
> This one is called indirectly by flush_dependencies so could be called
> for unloaded metaspaces.  I hadn't removed any to start out.

Ok.
Ship it!

/Mikael

> 
> > It seems to me that the core of the change is to not check the
> > _unloading list in CLDG::contains and then change all calls to go
> > through CLDG::contains, the CLD and then the SpaceManagers and Chunks,
> > that seems like a reasonable approach.
> > 
> > I like that you put CLDG::contains in PRODUCT in os::print_location!
> 
> Yes, this could be improved to ask is_valid_method or
> is_valid_constant_pool or the other metadata objects and call print, but
> many metaspace objects don't have vtables, so we'd have to check directly.
> 
> > The old code incorrectly calls the static Metaspace::contains for each
> > CLD in CLDG::contains but calls it like metaspace_or_null->contains(x)
> > so it's not strange that it was found to be slow :)
> 
> Yes, the old code was wrong and so one of them had to go.
> 
> Thanks,
> Coleen
> 
> > /Mikael
> > 
> > > bug link https://bugs.openjdk.java.net/browse/JDK-8029178
> > > 
> > > 
> > > 
> > > Thanks,
> > > 
> > > Coleen