RFR 8029567: Clean up linkResolver code

[Coleen Phillimore]

Please see new webrev with changes requested by Jiangli and Lois.

open webrev at http://cr.openjdk.java.net/~coleenp/8029567.02/

Thanks,
Coleen

On 5/28/15 4:03 PM, Coleen Phillimore wrote:
>
> Lois, Thank you for looking at this code closely, especially as one of 
> the experts in this code.
>
> On 5/28/15 10:52 AM, Lois Foltan wrote:
>>
>> On 5/14/2015 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
>>
>> Hi Coleen,
>>
>> Looks like a great clean up, few comments, nothing major though:
>
> Thanks - I'm hoping this makes this code easier for all of us.
>>
>> src/share/vm/classfile/loaderConstraints.[c/h]pp, 
>> systemDictionary.[c/h]pp
>> - I would prefer consistently changing the parameter name as well to 
>> "loader_data".  I associate "loader" with a ClassLoader.
>>
>
> Okay, I changed this.
>> src/share/vm/interpreter/linkResolver.hpp
>> - line #153 - would you consider moving the body of method_string 
>> into the hpp file since it is straight forward.
>
> I could do this since linkResolver.hpp does include method.hpp but it 
> would add another dependency on having to include method.hpp for the 
> call to Method->name_and_sig_as_C_string() and it doesn't save any 
> performance because it doesn't need to be inlined because it's in an 
> error message.
>
>> - Can you add a LinkInfo::print() method?
>
> Yes, I added one and will test it out.
>
> (gdb) call link_info.print()
> Link resolved_klass=java/lang/String$CaseInsensitiveComparator 
> name=<init> signature=(Ljava/lang/String$1;)V 
> current_klass=java/lang/String check_access=true
>
> from JavaCalls::call_static
>
> 254      LinkInfo link_info(klass, name, signature, KlassHandle(), 
> /*check_access*/false);
> (gdb)
> 255      LinkResolver::resolve_static_call(callinfo, link_info, true, 
> CHECK);
> (gdb) print link_info.print()
> Link resolved_klass=java/lang/ClassLoader name=findNative 
> signature=(Ljava/lang/ClassLoader;Ljava/lang/String;)J 
> current_klass=(none) check_access=false
>
>>
>>
>> src/share/vm/interpreter/linkResolver.cpp
>> - line #264 - I like the readability of returning a methodHandle 
>> result instead of void in LinkResolver::lookup_method_in_klasses, but 
>> just for my clarifications the result was being passed in by 
>> reference to avoid a copy ctor.  Now, there is a copy ctor occurring 
>> on the return value, correct?
>
> Returning things that need copy constructors are a common (maybe 
> standard, according to Kim) optimization in C++ called RVO - return 
> value optimization.
>
> I did analyze the .s files to make sure there were less destructors 
> (from copy constructors and assignment operators) called.
>
>>
>> - line #496, LinkResolver::resolve_method_statically formerly passed 
>> both resolved_method and resolved_klass by reference.  So changing 
>> your change implies that resolved_klass no longer needs the changes 
>> to be reflected in it post call?  For example, the 
>> Bytecodes::_invokedynamic if statement does reassign resolved_klass 
>> and returns with resolved_klass having that new value?
>
> Yes, the resolved_klass is not needed by the callers after the call.  
> Having it read-write seemed like an invitation to bugs to me.
>>
>> - line #726 why set current_klass if link_info.check_access() is 
>> false, could move that assignment into the if statement.  Or in line 
>> #753, instead of using link_info().current_klass() use "current_klass".
>>
>
> Okay, I made both use link_info.current_class().
>
>> - line #876 - same sort of comment as line #496 - resolved_klass 
>> could have been altered and the value changed post call.  So I  am 
>> assuming that is why the assert at line #889
>
> Yes, I added an assert because I couldn't prove this by inspection or 
> construction.  I didn't hit the assertion in any of the testing that I 
> did.
>>
>> src/share/vm/oops/klassVtable.cpp
>> - line 447 & 449, 1168 & 1170 -  can you rename loader1 and loader2 
>> to loader1_name and loader2_name, this is a nit comment but I find 
>> the use of a variable named "loader" to be very overloaded.
>>
>
> Yes, I also made your suggested change in the similar code 
> linkResolver.  This code was too difficult to consolidate in this 
> cleanup unfortunately.
>
> Thanks!
> Coleen
>> Thanks,
>> Lois
>>
>>>
>>> Thanks,
>>> Coleen
>>
>