RFR(S): 8024368: private methods are allocated vtable slots

[serguei.spitsyn at oracle.com]

Hi Dean,

Thank you for looking at it and the question!
Karen told that it is was a mistake back in 2005
to add non-final private methods into vtable.
I don't know anything about compatibility here and related tests.
Keren is traveling now and will be able to clarify it next week only.

Thanks,
Serguei


On 10/19/18 12:07 PM, dean.long at oracle.com wrote:
> It sounds like the vtable slot was there for backwards compatibility, 
> but I don't understand how a vtable slot for a private method ensured 
> compatibility (how does it affect behavior?), so I don't understand 
> how we continue to ensure backwards compatibility without it.  Also, 
> which tests check for the desired backwards compatibility?
>
> dl
>
> On 10/19/18 9:43 AM, serguei.spitsyn at oracle.com wrote:
>> Hi Karen and Lois,
>>
>> I have just one official review from David.
>> May I count your internal reviews as official?
>> Do we need the Compiler team to review this as well?
>>
>> Thanks,
>> Serguei
>>
>> On 10/16/18 18:29, David Holmes wrote:
>>> Bizarre!
>>>
>>> Looks good - officially Reviewed again!
>>>
>>> Thanks for fixing Serguei!
>>>
>>> David
>>>
>>> On 17/10/2018 4:34 AM, serguei.spitsyn at oracle.com wrote:
>>>> Re-sending my RFR.
>>>> Behive managed not to deliver it to the target mailing lists.
>>>> David got it because he was on the cc-list.
>>>>
>>>>
>>>> Please, review a fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8024368
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2018/8024368-priv-vtable.2/ 
>>>>
>>>>
>>>>
>>>> Summary:
>>>>
>>>>    Private method should not allocate vtable indeces.
>>>>    The fix tweaks several conditions in the functions:
>>>> klassVtable::update_inherited_vtable() and 
>>>> klassVtable::needs_new_vtable_entry(),
>>>>    adds a couple of asserts and makes several comments up-to-date.
>>>>
>>>>    The fix was preliminary reviewed by Karen, David H. and Lois.
>>>>
>>>> Testing:
>>>>
>>>>    The fix was verified with 
>>>> runtime/{appcds+Nestmates+RedefineTests+SelectionResolution}
>>>>    and vmTestbase/vm/runtime/defmeth tests.
>>>>    Also, I ran a mach5 job with the flags: 
>>>> tier1,tier2,hs-tier3,hs-tier4,hs-tier5.
>>>>
>>>>    By suggestion from David, it was also tested with the -Xlog 
>>>> output for vtables before and after the fix.
>>>>    I've done this comparison (awk was used to get rid of the first 
>>>> column with the time stamps).
>>>>    It shows that private methods were included into vtable before 
>>>> the fix but not included after.
>>>>    Please, see one fragment as an example:
>>>>
>>>>     166c166
>>>>     <  copy vtable from java.lang.Object to 
>>>> java.lang.StackFrameInfo size 17
>>>>     ---
>>>>     >  copy vtable from java.lang.Object to 
>>>> java.lang.StackFrameInfo size 16
>>>>     180d179
>>>>     <  adding 
>>>> java.lang.StackFrameInfo.ensureRetainClassRefEnabled()V at index 
>>>> 16, flags: private
>>>>     182c181
>>>>     <  copy vtable from java.lang.StackFrameInfo to 
>>>> java.lang.LiveStackFrameInfo size 17
>>>>     ---
>>>>     >  copy vtable from java.lang.StackFrameInfo to 
>>>> java.lang.LiveStackFrameInfo size 16
>>>>     188,201c187,191
>>>>     <  copy vtable from java.lang.Object to 
>>>> java.lang.StackStreamFactory$AbstractStackWalker size 18
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.close()V at index 
>>>> 5, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.toStackWalkMode(Ljava/lang/StackWalker;I)I 
>>>> at index 6, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.consumeFrames()Ljava/lang/Object; 
>>>> at index 7, flags: protected abstract
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.initFrameBuffer()V 
>>>> at index 8, flags: protected abstract
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.batchSize(I)I at 
>>>> index 9, flags: protected abstract
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.getNextBatchSize()I 
>>>> at index 10, flags: protected
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.skipReflectionFrames()Z 
>>>> at index 11, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.doStackWalk(JIIII)Ljava/lang/Object; 
>>>> at index 12, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.getNextBatch()I at 
>>>> index 13, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.beginStackWalk()Ljava/lang/Object; 
>>>> at index 14, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.fetchStackFrames(I)I 
>>>> at index 15, flags: private
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.fetchStackFrames(JJII[Ljava/lang/Object;)I 
>>>> at index 16, flags: private native
>>>>     <  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.callStackWalk(JIII[Ljava/lang/Object;)Ljava/lang/Object; 
>>>> at index 17, flags: private native
>>>>     ---
>>>>     >  copy vtable from java.lang.Object to 
>>>> java.lang.StackStreamFactory$AbstractStackWalker size 9
>>>>     >  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.consumeFrames()Ljava/lang/Object; 
>>>> at index 5, flags: protected abstract
>>>>     >  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.initFrameBuffer()V 
>>>> at index 6, flags: protected abstract
>>>>     >  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.batchSize(I)I at 
>>>> index 7, flags: protected abstract
>>>>     >  adding 
>>>> java.lang.StackStreamFactory$AbstractStackWalker.getNextBatchSize()I 
>>>> at index 8, flags: protected
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>
>