RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times

[Harold Seigel]

Thanks Coleen!

I'll add the comment before pushing the change.

Harold

On 3/26/2019 7:21 PM, coleen.phillimore at oracle.com wrote:
>
> Harold,
> This looks good!
>
> http://cr.openjdk.java.net/~hseigel/bug_8059357.2/webrev/src/hotspot/share/classfile/verifier.hpp.frames.html 
>
>
> Can you add a comment about this hashtable, ie, that it's indexed by 
> the constant pool index?
>
> 271 typedef ResourceHashtable<int, sig_as_verification_types*,
> 272 primitive_hash<int>, primitive_equals<int>, 1007>
> 273 method_signatures_table_type;
>
>
> Thanks,
> Coleen
>
> On 3/26/19 8:59 AM, Harold Seigel wrote:
>> Hi Ioi,
>>
>> Please review this updated webrev:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8059357.2/webrev/index.html
>>
>> It contains the changes you suggest below including calling 
>> translate_signature() on demand, avoiding the extra iteration through 
>> the constant pool.  I also increased the hashtable size to 1007 after 
>> discussing it with Coleen.
>>
>> Thanks! Harold
>>
>> On 3/22/2019 3:10 PM, Ioi Lam wrote:
>>> Hi Harold,
>>>
>>> I have one more comment. I see that the hashtable size is 71. I 
>>> wonder if this might be too small for bigger classes. Have you tried 
>>> loading all classes in the JDK to see how many method signatures 
>>> they use?
>>>
>>> I think the theoretical maximum number of method signatures is 
>>> around 65536 / 3, if you have a class with a big method that does this:
>>>
>>> class Foo {
>>>     void test() {
>>>        Bar.m(I);
>>>        Bar.m(L);
>>>        Bar.m(II);
>>>        Bar.m(IL);
>>>        Bar.m(LI);
>>>        Bar.m(LL);
>>>        Bar.m(III);
>>>        Bar.m(IIL);
>>>        ...
>>>
>>> Each MethodRef uses the same class and name, but a different 
>>> signature. So each call needs
>>>
>>>      1 x MethodRef
>>>      1 x Symbol (signature)
>>>      1 x NameAndType
>>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 3/22/19 11:16 AM, Harold Seigel wrote:
>>>> Hi Ioi,
>>>>
>>>> Thanks for looking at this!
>>>>
>>>> See comments inline.
>>>>
>>>> On 3/22/2019 1:41 PM, Ioi Lam wrote:
>>>>> Hi Harold,
>>>>>
>>>>> This looks good overall. Thanks for making the verifier faster!
>>>>>
>>>>> Some comments:
>>>>>
>>>>> 2831   sig_as_verification_types* mth_sig_verif_types = 
>>>>> *(method_signatures_table()->get(sig_index));
>>>>>
>>>>> How about calling the "get" method in a helper function, which 
>>>>> asserts that the returned value is not NULL?
>>>> If I call translate_signature() on demand then the get(sig_index) 
>>>> call will validly return NULL sometimes.
>>>>>
>>>>> 2999     for (int i = nargs; i < sig_verif_types_len; i++) {
>>>>> 3000 current_frame->push_stack(sig_verif_types->at(i), 
>>>>> CHECK_VERIFY(this));
>>>>>
>>>>> For added safety, how about asserting that the second item, if 
>>>>> exists, must be ITEM_Long_2nd or ITEM_Double_2nd?
>>>> Will do.
>>>>>
>>>>> Also, is there any reason why 
>>>>> ClassVerifier::init_method_sigs_table must be done at the 
>>>>> beginning? Is it possible to do the translate_signature() on 
>>>>> demand? That way you'd save one loop overate each element in the 
>>>>> constant pool.
>>>> Yes it is a good idea to call translate_signature() on demand. I'll 
>>>> give it a try.  It will also allowing getting rid of 
>>>> method_sig_count().
>>>>>
>>>>> If you decide to keep the separate loop, then maybe 
>>>>> unique_sig_indexes doesn't need to be a growable array, because 
>>>>> you already counted the _klass->method_sig_count()?
>>>>
>>>> I used a GrowableArray to take advantage of its append_if_missing() 
>>>> function but that won't be needed if done on demand.
>>>>
>>>> Thanks! Harold
>>>>
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>> On 3/22/19 7:43 AM, Harold Seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this fix for JDK-8059357.  This fix improves how 
>>>>>> the verifier translates method signatures into verification 
>>>>>> types. Previously, the verifier did a separate signature 
>>>>>> translation for every invoke* instruction even if the method 
>>>>>> signature was the same as a previously translated signature. With 
>>>>>> this change, the verifier, at the beginning of class 
>>>>>> verification, loops through the class's constant pool and 
>>>>>> translates each of the class's unique method signatures just 
>>>>>> once.  Each signature's resulting verification types are stored 
>>>>>> as an entry in a hash table indexed by the signature's 
>>>>>> method_ref's Utf8 constant pool index.
>>>>>>
>>>>>> When verifying an invoke* instruction, the list of verification 
>>>>>> types is retrieved from the hash table and stack argument 
>>>>>> assignability is checked without having to translate the 
>>>>>> signature into its verification types. Return types are handled 
>>>>>> similarly.
>>>>>>
>>>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8059357/webrev/
>>>>>>
>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8059357
>>>>>>
>>>>>> The fix was regression tested by running Mach5 tiers 1 and 2 
>>>>>> tests and builds on Linux-x64, Windows, and Mac OS X, Mach5 tiers 
>>>>>> 3 -5 on Linux-x64, and by running JCK-13 Lang and VM tests on 
>>>>>> Linux-x64.
>>>>>>
>>>>>> (Thanks to Eric Caspole for running performance tests on this 
>>>>>> change.)
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>
>>>
>