RFR 8029567: Clean up linkResolver code

[Jiangli Zhou]

Hi Coleen,

Sounds good.

Thanks,
Jiangli

On May 29, 2015, at 3:17 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:

> 
> 
> On 5/29/15 6:02 PM, Jiangli Zhou wrote:
>> Hi Coleen,
>> 
>> Looks good. The only suggestion I have is to remove the explicit '/*check_access*/true’ when creating LinkInfo in ciEnv::lookup_method(), ciEnv.cpp line 712.
> 
> Thanks, Jiangli,
> I thought since the changes were small, I couldn't possibly miss anything so already checked it in.  I'll get this line next time.
> Coleen
> 
>> 
>> 
>> Thanks,
>> Jiangli
>> 
>> On May 28, 2015, at 3:34 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>> 
>>> The change (jck testing in progress) with the loader constraint changes backed out.
>>> 
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8029567.03/
>>> 
>>> Thanks,
>>> Coleen
>>> 
>>> On 5/28/15 5:59 PM, Coleen Phillimore wrote:
>>>> Hi Karen,
>>>> 
>>>> Yes, I'll back out the loader constraint changes.  I was trying to merge the error message construction code for failing loader constraint checks since it's similar code in 5 places but I gave up, and made the CLD change as a side effect.
>>>> We store ClassLoaderData in the loader constraint table now but this is a good find, because I think there is a behavior change here in the comparison.
>>>> 
>>>> Thanks!
>>>> Coleen
>>>> 
>>>> On 5/28/15 5:43 PM, Karen Kinnear wrote:
>>>>> Coleen,
>>>>> 
>>>>> 1. Thank you for highlighting where we have check_access as false - so we can make sure we
>>>>> really really meant to do that going forward.
>>>>> 
>>>>> 2. I am concerned about the loader constraint changes to use class loader data rather than class loaders.
>>>>> Since these are not 1:1 - what about use cases in which the class loaders are the same but the loader datas are not?
>>>>> 
>>>>> In the Valhalla exploration of ClassDynamic, we are exploring having dynamic classes, which would be defined
>>>>> via DefineAnonymousClass - which can be used in signatures, so we would need to be able to compare types which may
>>>>> have different loader datas but might need to compare as equivalent since they have the same class loader.
>>>>> 
>>>>> I believe the intention of this set of changes is cleanup - not meant to change behavior.
>>>>> 
>>>>> But that change actually is a meaningful behavior change - and I think it is premature to make it.
>>>>> 
>>>>> Could you possibly back out the loader constraint related changes?
>>>>> 
>>>>> Those will take more thought, more future designs, and detailed targeted tests.
>>>>> 
>>>>> thanks,
>>>>> Karen
>>>>> 
>>>>> On May 14, 2015, at 7:16 PM, Coleen Phillimore wrote:
>>>>> 
>>>>>> Summary: Moved non-const reference return values to actual return values, refactored error handling code, pass CLD rather than class_loader oop, remove oop from Method* variable names.
>>>>>> 
>>>>>> Also modified the long parameter lists into a class LinkInfo to hold information from the constant pool that is always passed through several layers of functions together.  Also reformatted to split parameter list lines to some reasonable width.
>>>>>> 
>>>>>> The type methodHandle should be passed as const reference types to avoid copy construction, because it has a non-trivial destructor. This sort of change could be made in more places in the JVM, but I stopped with linkResolver.
>>>>>> 
>>>>>> Ran all hotspot jtreg tests, jck lang/vm/api/java_lang tests, internal testbase tests: vm.quick.testlist, vm.defmeth.testlist, proposed new selection-resolution tests, jdk/java/lang/invoke tests (see RFR).
>>>>>> 
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8029567/
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8029567
>>>>>> 
>>>>>> Thanks,
>>>>>> Coleen
>