RFR: 8013635 Lambda: vm conditionally create bridges for generic signatures
Karen Kinnear
karen.kinnear at oracle.com
Thu Jun 27 10:10:34 PDT 2013
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
>
More information about the hotspot-runtime-dev
mailing list