RFR: 8013635 Lambda: vm conditionally create bridges for generic signatures

[serguei.spitsyn at oracle.com]

The updated version looks good.

Thanks,
Serguei


On 6/27/13 10:10 AM, Karen Kinnear wrote:
> Many thanks - especially since you know this code better than I do :-)
> And it is much better now due to Coleen and Serguei's review comments.
>
> thanks,
> Karen
>
> On Jun 27, 2013, at 11:34 AM, Bharadwaj Yadavalli wrote:
>
>> The updated version looks good.
>>
>> Thanks,
>>
>> Bharadwaj
>>
>> On 6/26/2013 10:52 PM, Karen Kinnear wrote:
>>> Coleen,
>>>
>>> I so appreciate the detailed review - many thanks!
>>>
>>> Updated webrev:
>>>
>>> http://cr.openjdk.java.net/~acorn/8013635.2/webrev/
>>>
>>> tested: with javac and metafactory changes:
>>> defmeth tests,
>>> jtreg jdk.util in progress
>>> vm.quick.testlist in progress
>>>
>>> On Jun 25, 2013, at 1:48 PM, Coleen Phillimore wrote:
>>>
>>>> Karen,
>>>>
>>>> http://cr.openjdk.java.net/~acorn/8013635.1/webrev/src/share/vm/classfile/defaultMethods.cpp.frames.html
>>>>
>>>> If someone calls this version of print_on(), won't it call the one at 440 and then crash because signature is null?
>>>>
>>>> 437   void print_on(outputStream* str) const {
>>>> 438     print_on(str, 0);
>>>> 439   }
>>>>
>>> Good catch.
>>> This is messier than I wanted - so I refactored and renamed some of the print_on's.
>>>> Shouldn't it just be removed?
>>>>
>>>> You could rename "curslotidx" to current_slot_index...
>>> Done.
>>>> 774     InstanceKlass* sub = current_depth() > 0 ? class_at_depth(1) : NULL;
>>>>
>>>> Variable sub is not used.
>>> Removed.
>>>> ShadowChecker::path_has_shadow() should be pure virtual because if you don't override ShadowChecker the visit function will never find a shadow.  line 1088.
>>>>
>>> Yes - thank you.
>>>> 1109       InstanceKlass* sub = class_at_depth(i + 1);
>>>>
>>>> Variable 'sub' is not used here (trying to understand this).
>>> Removed - sorry - left over from the refactoring.
>>>> 1097 // Only called if the resolved_klass is a direct superinterface of
>>>> 1098 // the caller's class.
>>>>
>>>>
>>>> This comment seems out of context, in that which is resolved_klass?   Should this comment be in the caller or the caller's caller.  It doesn't seem helpful here to understand what this is doing or why.   Except:
>>> Removed - and clarified "...candidates which are methods in immediate superinterfaces" - which
>>> the caller already checks for
>>>> 1111       if (ik->is_interface()) {
>>>>
>>>> And why does this only check for if an interface class shadows the method?  I thought a method of a non-interface class can also shadow a method?
>>> We are walking up the superinterfaces, so once you get to an interface, the only non-superinterface is j.l.Object which shouldn't
>>> be providing default methods.
>>>> 1307   if (!direction->is_interface()) {
>>>> 1308     // We should not be here
>>>> 1309     return NULL;
>>>> 1310   }
>>>>
>>>> Should this be an assert instead?   The caller of find_super_default won't be happy to get a NULL back without an exception pending.
>>> Good catch. Changed to assert, and renamed "direction" to super_class.
>>>> I don't have any other comments.  This looks good!
>>>>
>>>> Coleen
>>> And thanks for the detailed discussion,
>>> Karen
>>>> On 06/20/2013 04:18 PM, Karen Kinnear wrote:
>>>>> Please review:
>>>>> https://jbs.oracle.com/bugs/browse/JDK-8013635: BridgeMethodsLinearTest.java fails in
>>>>> Lambda nightly runs
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~acorn/8013635.1/webrev/
>>>>>
>>>>> This particular set of changes modifies the vm to conditionally handle generic signatures
>>>>> when processing default methods. Basically this code refactors the defaultMethods logic
>>>>> to work either with generic signatures or only with erased signatures.
>>>>>
>>>>> The flag: ParseGenericDefaults is set to "true" in this webrev to continue to work with
>>>>> the current javac and lambda metafactory components.
>>>>>
>>>>> The intention is to change ParseGenericDefaults to false and simultaneously change
>>>>> javac to generate bridges to handle generic signatures in interface methods, and
>>>>> the metafactory to no longer generate bridges.
>>>>> With the flag change, the vm will no longer generate bridges for generic
>>>>> signatures.
>>>>>
>>>>> A subsequent fix: 8012294 will completely remove the vm generic interface handling
>>>>> for jdk8.
>>>>>
>>>>> Please approve this change with both flag settings, so we can coordinate making
>>>>> those changes in the order that is least disruptive.
>>>>>
>>>>> Testing:
>>>>>
>>>>> I. hotspot-rt: ParseGenericDefaults=true:
>>>>> jprt
>>>>> vm.quick.testlist
>>>>> jdk-jtreg - in progress
>>>>> jck-lang, vm - in progress
>>>>>
>>>>> II. Lambda repo: ParseGenericDefaults=false:
>>>>> These two files, with other changes not currently needed - tested in lambda repository for a month
>>>>>     including tl nightlies and updated BridgeMethodsLinearTest
>>>>>
>>>>> III. Current hotspot-rt with these two files only and ParseGenericDefaults=false
>>>>> with jdk8tl repository with 8015402: metafactory changes and 8013789: javac changes:
>>>>>
>>>>> vm.quick.testlist - in progress
>>>>> jdk-jtreg - in progress
>>>>> jck-lang, vm - in progress
>>>>>
>>>>> thanks,
>>>>> Karen