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

[Markus Grönlund]

Hi Coleen,

I have taken a look as well as running the nashorn crypto tests with the patches - these changes looks good.

Thanks for addressing this.

/Markus


-----Original Message-----
From: Coleen Phillimore 
Sent: den 13 maj 2014 22:20
To: Mikael Gerdin
Cc: hotspot-dev at openjdk.java.net
Subject: Re: RFR 8038212: Method::is_valid_method() check has performance regression impact for stackwalking


On 5/13/14, 3:38 PM, Mikael Gerdin wrote:
> 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".

We actually already test that classes are Metaspace::contains() in the system dictionary verification at the end of the debug build (Dictionary::verify => instanceKlass::verify => Metaspace::contains)

I think this test adds nothing now and given your concerns, I'm going to revert it.

Thanks,
Coleen

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