RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times

[Ioi Lam]

Hi Harold, this looks good.

Just one minor suggestion. The code from lines 2799~2812 seems to be 
self-contained, and might be a good idea to move into a separate 
function. No need for new webrev if you decide to do this.

Thanks
- Ioi

On 3/26/19 5: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
>>>>>
>>>>
>>