RFR 8038212: Method::is_valid_method() check has performance regression impact for stackwalking

[Mikael Gerdin]

On Tuesday 13 May 2014 14.49.03 Coleen Phillimore wrote:
> On 5/13/14, 2:41 PM, Mikael Gerdin wrote:
> > On Tuesday 13 May 2014 14.16.30 Coleen Phillimore wrote:
> >> Hi again,
> >> 
> >> I have moved the metaspace vm internal test to the end of execution, and
> >> also test if klasses are in metaspace, which works fine.  Thank you for
> > 
> >> the suggestion!  Here is a new webrev:
> > I'm not too happy about moving the test to the before_exit handler.
> 
> Why not?

I guess that most of the other unit tests are (and in my opinion should be) as  
independent as possible of JVM state such as loaded classes. 
I was fine with the test before since it's a reasonably small amount of code to 
get a test and it adds some test coverage.
When the change involves adding additional hooks for unit tests in the exit 
path I'm just slightly more bothered by it.

If we want a proper unit test for Metaspace::contains which tests all sorts of 
alllocations/classloaders and such then it needs to be a bit more explicit 
with setting up a known test environment.

> 
> > Also, how can using the run_unit_test-macro (defined in jni.cpp) even work
> > in java.cpp?
> 
> It's moved to globalDefinitions.hpp.

I see.

> 
> > About methods and classes being available I think we may have preloaded
> > some classes in Threads::create_vm, right?
> 
> Yes, we have loaded classes before the initial ExecuteVMInternalTests
> but they are only in the boot class loader, which is only in one
> metaspace.  So it's not much of a test.  At least at the end, we will
> have classes in the application class loader that we are testing whether
> they are contained in metaspace.

Ok.

> 
> I am not motivated to add the test for just for the sake of having a
> test.   This change fixes the nashorn performance problem.  Maybe that's
> test enough.
> 
> before my change:  [crypto] 1273 ops/minute (502-1511), warmup=141
> after my change:     [crypto] 8548 ops/minute (5861-9017), warmup=960

Well, I guess the test isn't really for the correctness of the ::contains 
function anyway? And we don't really have a good way to do a measured test to 
make sure that ::contains isn't "too slow".

/Mikael

> 
> Coleen
> 
> > /Mikael
> > 
> >> http://cr.openjdk.java.net/~coleenp/8038212_2/
> >> 
> >> Thanks,
> >> Coleen
> >> 
> >> On 5/13/14, 9:51 AM, Coleen Phillimore wrote:
> >>> Hi Mikael,
> >>> 
> >>> On 5/13/14, 6:33 AM, Mikael Gerdin wrote:
> >>>> Hi Coleen,
> >>>> 
> >>>> On Monday 12 May 2014 20.19.15 Coleen Phillimore wrote:
> >>>>> Summary: Only prune metaspace virtual spaces at safepoint so walking
> >>>>> them is safe outside a safepoint.
> >>>>> 
> >>>>> Walking class loader data graph for contains is really slow for
> >>>>> applications like nashorn that has a lot of class loader data.
> >>>>> 
> >>>>> Tested with nsk.quick.testlist, jtreg tests, and jck tests. Also ran
> >>>>> 
> >>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8038212/
> >>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8038212
> >>>> 
> >>>> Thanks for taking care of this fix.
> >>>> I think the idea of deferring CLDG::purge to the next safepoint
> >>>> cleanup is
> >>>> sound.
> >>> 
> >>> Oh great!  I'm glad you agree.
> >>> 
> >>>> I like the removal of CLDG::contains since it makes more sense to ask
> >>>> Metaspace straight up.
> >>>> 
> >>>> I have a small question about the test you added:
> >>>> 
> >>>> 3798   static void test_contains() {
> >>>> 3799     // Test that all the methods in the CLDG are contained
> >>>> 3800     ClassLoaderDataGraph::methods_do(assert_contains);
> >>>> 3801   }
> >>>> 
> >>>> Have we always allocated methods at this point in the VM startup?
> >>> 
> >>> I thought this ExecuteVMInternalTests (or whatever the option is
> >>> called) is at shutdown?  I'll check.
> >>> 
> >>>> Should we also test CLDG::classes_do to make sure it works for the
> >>>> compressed
> >>>> class space?
> >>> 
> >>> I had that same thought this morning.  Yes, I'll add it.
> >>> 
> >>> Coleen
> >>> 
> >>>> /Mikael
> >>>> 
> >>>>> Marcus L. tested it with nashorn tests.
> >>>>> 
> >>>>> Thanks,
> >>>>> Coleen