RFR 8029567: Clean up linkResolver code

[Coleen Phillimore]

Jiangli,  Thank you for reviewing my change.

On 5/26/15 10:15 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> I like the refactor changes. The code looks much cleaner. Here are 
> some of my comments.
>
> *LinkInfo:*
> ciEnv::lookup_method(), ciField::will_link() and 
> ciMethod::resolve_vtable_index() create LinkInfo with ‘_check_access’ 
> set to true. It might be better to make ‘_check_access’ default to 
> ‘true’ in the LinkInfo constructor.

Opinion comments are welcome because this is a cleanup change but I went 
back and forth on this, and I really want the caller of the LinkInfo 
constructor to explicitly decide whether access checks should be 
performed.   The language rules deciding this are daunting so I didn't 
want to allow for mistakes.
> *
> *
> *Bytecode_invoke::static_target(), bytecode.cpp:*
> Bytecode_invoke::static_target() originally returns a null handle 
> instead of ‘NULL’ if the method is not resolved. Now that’s changed, 
> however caller of Bytecode_invoke::static_target() still expects null 
> handle instead of ’NULL’.
>
> *LinkResolver::lookup_method_in_klasses(), linkResolver.cpp:*
> Same as Bytecode_invoke::static_target(). This now might return ‘NULL’ 
> instead of null handle.
>
https://bugs.openjdk.java.net/browse/JDK-8066624

This code for CHECK_(nullHandle) is my fault because of an experiment I 
was doing years ago by disallowing the default conversion from oop to 
Handle.  The experiment was because when converting to a Handle, the 
function Thread::current() was being called implicitly when in most 
cases THREAD is available and showed up in somebody's profiling data.

Because the conversion from NULL to any Handle does not call 
Thread::current() and it's allowed, I think CHECK_NULL looks a lot 
better in the code than CHECK_(methodHandle()).

I changed the code to this though:

methodHandle Bytecode_invoke::static_target(TRAPS) {
   constantPoolHandle constants(THREAD, this->constants());

   Bytecodes::Code bc = invoke_code();
   return LinkResolver::resolve_method_statically(bc, constants, 
index(), THREAD);
}

Because CHECK_NULL in a return statement does something bad that I can't 
remember.
> The lookup_polymorphic_method() and resolve_method() have the similar 
> issue. It probably is better to make all those return ‘null handle’ 
> like they originally do.
>
> *Indentation:*
> loaderConstraints.hpp line 68
> loaderConstraints.cpp line 74
> linkResolver.cpp line 440 - 444
> systemDictionary.cpp line 2238 - 2239

I fixed these.  My window width for code is 100 so I tried not to wrap 
for lines longer than that.

Thanks!
Coleen
>
> Thanks,
> Jiangli
>
> On May 14, 2015, at 4:16 PM, Coleen Phillimore 
> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>> 
> 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/ 
>> <http://cr.openjdk.java.net/%7Ecoleenp/8029567/>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8029567
>>
>> Thanks,
>> Coleen
>