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

[David Holmes]

Hi Coleen,

"actually S"? :)

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

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. ??

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.

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

---

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.

---

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.

---

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.

---

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.

---

src/share/vm/oops/cpCache.cpp

Nit: objArrayHandle resolved_references (Thread::current(), ...

Please remove space before (

---

src/share/vm/prims/jvm.cpp

Nit: need to fix indent on StackWalk::walk call second line.

---

src/share/vm/prims/jvmtiGetLoadedClasses.cpp

Ditto re _thread -> _cur_thread

---

src/share/vm/prims/jvmtiImpl.cpp

Nit: space before ( again (on handle constructor)

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

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