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

[Coleen Phillimore]

Thank you Harold!
Coleen

On 5/14/14, 8:50 AM, harold seigel wrote:
> Hi Coleen,
>
> Your changes look good to me, also.
>
> Harold
>
> On 5/14/2014 5:10 AM, Markus Grönlund wrote:
>> 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
>