RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times

[Harold Seigel]

Thanks Ioi!

I decided to leave the lines ‘as is’ for now.

Harold

> On Mar 26, 2019, at 8:00 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 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
>>>>>> 
>>>>> 
>>> 
>