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

[Coleen Phillimore]

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?
>
> Also, how can using the run_unit_test-macro (defined in jni.cpp) even work in
> java.cpp?

It's moved to globalDefinitions.hpp.
>
> 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.

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

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