RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times

[Ioi Lam]

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