RFR 8029567: Clean up linkResolver code

[Coleen Phillimore]

On 5/27/15 5:24 PM, Karen Kinnear wrote:
> 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.

Ok, that is fine.  I can make it the default.  When called through 
javaCalls, we pass false to check_access which I thought was surprising.

Thanks,
Coleen
>
> 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