RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times

[Harold Seigel]

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