[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR 8059357: ClassVerifier redundantly checks constant pool entries multiple times
From:       Harold Seigel <harold.seigel () oracle ! com>
Date:       2019-03-27 17:48:05
Message-ID: A1F9A5C4-1EE4-40ED-877F-B2B56D14E76A () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]