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

[Coleen Phillmore]

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.
>
>
> 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
>