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

[harold seigel]

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