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

[Coleen Phillimore]

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