RFR 8029567: Clean up linkResolver code

[Coleen Phillimore]

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
>