RFR 8029567: Clean up linkResolver code

[Karen Kinnear]

Coleen, Jiangli,


On May 27, 2015, at 3:26 PM, Coleen Phillimore wrote:

> 
> 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.
I will add my opinion that check_access should default to true. It should be a very very rare caller that turns it off and we want to highlight
that in code reviews.

thanks,
Karen

>> *
>> *
>> *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
>> 
>