(10) RFR (actually S) 8169881: Remove implicit Handle conversions oop->Handle

[Coleen Phillimore]

On 2/13/17 8:44 PM, David Holmes wrote:
> Hi Coleen,
>
> "actually S"? :)

Well, maybe not but it shouldn't have a lot of merge conflicts for 
backports.

>
> On 14/02/2017 6:10 AM, Coleen Phillimore wrote:
>> Summary: Pass THREAD to Handle as argument instead of implicit
>> Thread::current() call.
>
> Well there's a bit more to it than that as you also change parameter 
> types so that we (sometimes) avoid:
>
> - oop -> Handle -> oop
> - Handle -> oop -> Handle

Yes, I put that in the bug but not in the RFR.  The recommendation is to 
Handle the oop as far up as possible in the call stack and pass Handle 
around.
>
> across method calls. This leads to some puzzling differences eg:
>
> src/share/vm/aot/aotCodeHeap.cpp
> src/share/vm/classfile/javaClasses.hpp
>
> The change in aotCodeHeap.cpp reverses the use of Handle and oop, but 
> it is unclear why Throwable::print and Throwable::print_stack_trace 
> have different signatures in that regard. They used to both take a 
> Handle but now one takes an oop instead. ??

Yes, you found the exceptions in Throwable.  A lot of the 
java_lang_Throwable (and other javaClasses.hpp classes) pass oop instead 
of Handle and I changed print to oop to make it consistent, and because 
they it being called from other functions in Throwable with oop.  
Throwable::print_stack_trace actually goes to a safepoint so has to be 
Handle.


>
> More generally it is unclear why you made the changes you did in 
> places - see below for some specifics. I'm left questioning how to 
> know when to take an oop and when to take a Handle. Of course that 
> question already exists, but these changes didn't make it any clearer 
> for me.

Pass a Handle early and often is still the rule.  For all but GC code.  
There are still some functions that are very small and don't safepoint 
and take oop.  Some of these have migrated to taking Handle, but some 
haven't.  I didn't want to make this change larger for now.

>
> I understand why the oop to Handle implicit conversion could be 
> problematic due to the Thread::current() calls, but I don't understand 
> why you couldn't keep (? add?) the Handle to oop implicit conversions ??

There weren't Handle -> oop implicit conversions.  The conversion back 
to oop uses the operator() which remains.

The oop -> Handle conversion has cost, both at runtime and also to the 
GC because it increases the number of Handles on the Thread->_handle_area.

I did this same experiment years ago when Thread::current() was at the 
top of the profiling call stack but back then I needed to add too many 
explicit Thread::current() calls.  There aren't many in this change, so 
it's worth doing.  Also, metadata handles have the same problem but 
there are still too many of them to remove the implicit conversion.   
And {instance}KlassHandle needs to be removed first.  They do nothing 
now but hide the type.

>
> ---
>
> src/share/vm/c1/c1_Runtime1.cpp
>
> Aside: I'm a little surprised that there was not a Handle assignment 
> operator or copy constructor such that given:
>
> 863   Handle appendix(THREAD, NULL);
> 970         appendix = cpce->appendix_if_resolved(pool);
>
> that it simply updated the NULL to the desired value, while keeping 
> the THREAD intact.

Handle doesn't save the THREAD so a NULL handle is just oop* NULL. The 
RHS of the expression has to be converted to Handle and the default 
assignment operator copies it to appendix.

>
> ---
>
> src/share/vm/ci/ciInstance.cpp
>
> Not at all obvious that replacing Handle with raw oop is correct. I'm 
> not saying it isn't, just that it isn't obvious - and I'll wonder why 
> the Handle was used in the first place.

It is safe because it's immediately unhandled in all the case 
statements, and I wanted to avoid a Thread::current call.
>
> ---
>
> src/share/vm/classfile/javaClasses.cpp
>
> Why change java_lang_String::as_symbol_or_null to take a Handle 
> instead of an oop if you are simply going to unwrap the Handle to get 
> the oop back. ??? Particular when a caller like 
> src/share/vm/prims/methodHandles.cpp has to create the Handle from oop 
> in the first place.

I changed it to be the same as java_lang_String::as_symbol(Handle 
java_string ...) which took a handle. java_lang_String::as_symbol() was 
the same - it immediately unhandles the argument to pass to the rest of 
the functions.  I changed java_lang_String::as_symbol() to take an oop 
to match, and there are few scattered changes from that.


>
> ---
>
> src/share/vm/jvmci/jvmciCompilerToVM.hpp
>
> It is not at all obvious that JavaArgumentUnboxers are 
> thread-confined! I'm also concerned by API's (unfortunately a number 
> pre-existing) that take a Thread/JavaThread argument but really 
> require it to be the current thread. To that end I'd rather see 
> _thread as _cur_thread and an assertion when it is set.

Since it's a ResourceObj and not StackObj (setting aside debate about 
that), I agree with you.  I changed it to use Thread::current() where it 
handles the object, which it explicitly did before.  In this case, it is 
a stack allocated thing but it's not guaranteed to be that.

>
> ---
>
> src/share/vm/oops/cpCache.cpp
>
> Nit: objArrayHandle resolved_references (Thread::current(), ...
>
> Please remove space before (

fixed.

>
> ---
>
> src/share/vm/prims/jvm.cpp
>
> Nit: need to fix indent on StackWalk::walk call second line.

fixed.

>
> ---
>
> src/share/vm/prims/jvmtiGetLoadedClasses.cpp
>
> Ditto re _thread -> _cur_thread
>

This one is a StackObj (a Closure which should be used that way). So I 
changed _thread to _cur_thread and added an assert that it == 
Thread::current() in the constructor.

> ---
>
> src/share/vm/prims/jvmtiImpl.cpp
>
> Nit: space before ( again (on handle constructor)

fixed.
>
>> It's a very small change to a number of files.  Notably the new JVMCI
>> files have a lot of implicit conversions from oop to handle. I want to
>> get this change in early for jdk10 to stop this usage.  I also added a
>> few HandleMarks where I found that the HandlesArea was growing large.
>
> The addition and removal of the HandleMarks seems problematic because 
> there are no obvious rules being applied. It is hard to know where and 
> when a HandleMark is needed or not. Looking at 
> src/share/vm/jvmci/jvmciCompilerToVM.cpp do we really want to 
> create/destroy the HandleMark on each loop iteration? That would only 
> seem necessary if we know the number of iterations is so high that we 
> might consume all our handle space.

Nobody knows when and where to put HandleMarks.   It's a dark art. I 
also was testing with a JVM that restored the assert if HandleArea got 
over 200 handles and the numbers got very high in these functions.  The 
GC has to walk these HandleAreas and I am trying to reduce work for them.

I tried to write about this in the bug.   I'd like to see if changing 
Handle to a proper constructor/destructor like methodHandles, so that 
only the Handles that are active need to be collected.   Then we don't 
need HandleMark and their associated mystery.   There are several 
HandleMarks in code that obviously doesn't allocate any Handles.  I took 
out several but restored them so that this patch was smaller.   To 
change Handle to a scoped object would require changing passing Handle 
as a const reference like I did with methodHandle which is too big of a 
change for early-jdk10.

The reason I worked on this RFE was because I was curious to see how 
many oops/Handles we're passing around and using from within the JVM 
these days.  It's more than I thought.

Thank you for reviewing this.  I'm going to rerun some tests with the 
changes above and send another webrev.

Coleen

>
> Thanks,
> David
> ------
>
>> See bug for more on the motivation of this change.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8169881.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8169881
>>
>> Tested by running all hotspot jtreg tests with -XX:+CheckUnhandledOops.
>> There weren't any unhandled oops amazingly.
>>
>> Thanks,
>> Coleen