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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Feb 16 01:46:16 UTC 2017

On 2/15/17 10:43 AM, coleen.phillimore at oracle.com wrote:

Hi Serguei, Thank you for reviewing this. On 2/15/17 2:18 AM, serguei.spitsyn at oracle.com wrote: Hi Coleen,

Thank you for the review discussion with David! It helps to understand the webrev details. It looks good in general. Some comments/questions below. http://cr.openjdk.java.net/~coleenp/8169881.02/webrev/src/share/vm/jvmci/jvmciCodeInstaller.cpp.udiff.html for (jint i = 0; i < values->length(); i++) { ScopeValue* cursecond = NULL; - Handle object = values->objat(i); - BasicType type = JVMCIRuntime::kindToBasicType(slotKinds->objat(i), CHECK); + Handle object (THREAD, values->objat(i)); + Handle slotkind (THREAD, slotKinds->objat(i)); + BasicType type = JVMCIRuntime::kindToBasicType(slotkind, CHECK); Do we need a HandleMark in the loop? Yes, I don't think it would hurt. Added. http://cr.openjdk.java.net/~coleenp/8169881.02/webrev/src/share/vm/jvmci/jvmciCompilerToVM.cpp.udiff.html Handle result; if (!klass.isnull()) { - result = CompilerToVM::getjvmcitype(klass, CHECKNULL); + oop resultoop = CompilerToVM::getjvmcitype(klass, CHECKNULL); + result = Handle(THREAD, resultoop); } else { result = javalangString::createfromsymbol(symbol, CHECKNULL); } return JNIHandles::makelocal(THREAD, result()); Not clear, why does the result need to be a handle. There's no reason this had to be a Handle, but javalangString::createfromsymbol returns a Handle. That's why. This seems better, agree? oop resultoop; if (!klass.isnull()) { oop resultoop = CompilerToVM::getjvmcitype(klass, CHECKNULL); } else { Handle result = javalangString::createfromsymbol(symbol, CHECKNULL); resultoop = result(); } return JNIHandles::makelocal(THREAD, resultoop);

Correction, I redeclared result_oop above.

oop result_oop; if (!klass.is_null()) { result_oop = CompilerToVM::get_jvmci_type(klass, CHECK_NULL); } else { Handle result = java_lang_String::create_from_symbol(symbol, CHECK_NULL); result_oop = result(); }

Coleen

http://cr.openjdk.java.net/~coleenp/8169881.02/webrev/src/share/vm/oops/instanceKlass.cpp.udiff.html + const char* javapkg = "java/"; . . . - strncmp(classname->asCstring(), JAVAPKG, JAVAPKGLEN) == 0) { + strncmp(classname->asCstring(), javapkg, strlen(javapkg)) == 0) { Why this change is needed? It is a reverse of the recent Rachel's fix. I messed this up in the integration. Thank you for finding it! This was the only diff in that function: @@ -2460,7 +2463,7 @@ TRAPS) { ResourceMark rm(THREAD); if (!classloader.isnull() && - !SystemDictionary::isplatformclassloader(classloader) && + !SystemDictionary::isplatformclassloader(classloader()) && classname != NULL && strncmp(classname->asCstring(), JAVAPKG, JAVAPKGLEN) == 0) { TempNewSymbol pkgname = InstanceKlass::packagefromname(classname, CHECK);

http://cr.openjdk.java.net/~coleenp/8169881.02/webrev/src/share/vm/oops/instanceKlass.cpp.frames.html 701 void InstanceKlass::initializeimpl(instanceKlassHandle thisk, TRAPS) { 702 HandleMark hm(THREAD); . . . 712 // refer to the JVM book page 47 for description of steps 713 // Step 1 714 { 715 HandleMark hm(THREAD); It is not clear what is the reason for one more HandleMark at 715. I'll delete the second one. I added it for debugging number of Handles. Thanks for reviewing! Coleen Thanks, Serguei On 2/14/17 14:21, coleen.phillimore at oracle.com wrote: New webrev: open webrev at http://cr.openjdk.java.net/~coleenp/8169881.02/webrev see coments below first. On 2/13/17 11:51 PM, David Holmes wrote: Hi Coleen,

On 14/02/2017 1:53 PM, Coleen Phillimore wrote:

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. I hear what you are saying but it is hard to actually see that reflected in the code. 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::printstacktrace 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 javalangThrowable (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::printstacktrace actually goes to a safepoint so has to be Handle. I think in javaClasses.hpp the rule should be oop unless Handle is definitely needed. It's easiest to keep it an oop because most of the functions just do an objfield, etc. oop javalangThrowable::message(oop throwable) { return throwable->objfield(detailMessageoffset); } but it's likely that the oop throwable is a handle up the call stack. Interestingly looking at it's callers is one in javaClasses.cpp that's an oop, one in exceptions.cpp that's a Handle, oop msg = javalangThrowable::message(exception()); And then there's the one in universe.cpp that's a naked oop, which can get moved before the call. // get the error object at the slot and set set it to NULL so that the // array isn't keeping it alive anymore. oop exc = preallocatedoutofmemoryerrors()->objat(next); assert(exc != NULL, "slot has been used already"); preallocatedoutofmemoryerrors()->objatput(next, NULL); <= This on G1 can take out a lock for the oopstore and GC and move the defaulterr // use the message from the default error oop msg = javalangThrowable::message(defaulterr); <= defaulterr is an oop. assert(msg != NULL, "no message"); javalangThrowable::setmessage(exc, msg); exc is a naked oop too. I don't think the naked oop detector triggers for objatput/oopstore because G1 wasn't in the code when it was added. Creating a Handle and passing it saves having to visually inspect functions to try to find naked oops. Fixing this to have a local handle though because the callers don't have THREAD.

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. Okay then it seems an implicit Handle -> oop would avoid the need to change the caller from foo to foo() if the underlying API switched from oop to Handle. Yes but it's better to just pass the oop along until something really needs to do something with it. The oop -> Handle conversion has cost, both at runtime and also to the GC because it increases the number of Handles on the Thread->handlearea. Sure. 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/c1Runtime1.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->appendixifresolved(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. I had assumed Handle saved the Thread. No, then it would be two fields. It's only one so passing it is cheap now. --- 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 javalangString::assymbolornull 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 javalangString::assymbol(Handle javastring ...) which took a handle. javalangString::assymbol() was the same - it immediately unhandles the argument to pass to the rest of the functions. I changed javalangString::assymbol() to take an oop to match, and there are few scattered changes from that. I have to disagree with this one. AFAICS there is no reason this: 517 Symbol* javalangString::assymbol(Handle javastring, TRAPS) { needs to take a Handle in the first place. The javastring is unwrapped to get the oop and the oop is only passed to a few other String methods and then not touched. Looking at the API's in javaClass.hpp the norm should be to take oop (as the bulk of methods do) with Handle only used when actually required. I changed this one to take oop because it was obvious on inspection but note that the "required" is sometimes really hard to see! Chasing naked oop problems isn't something we do very much anymore because we converted many arguments to Handles. Note that the runtime only passes oops much of the time and doesn't usually do anything with them. The exception is javaClasses.cpp though.

--- 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 curthread 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. Ok. --- src/share/vm/oops/cpCache.cpp Nit: objArrayHandle resolvedreferences (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 -> curthread This one is a StackObj (a Closure which should be used that way). So I changed thread to curthread and added an assert that it == Thread::current() in the constructor. Thanks. --- 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 So a trade-off between too much cleanup overhead if we have too many, and too much GC overhead if we have too few. And no real way to know either way - Ouch! :( 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. Ok. This sounds like a way forward though. In combination with an easy way to detect when handles are required, this would allow very precise management of handle lifetimes. Good, I'm glad you think so too. 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. Ok. Thanks. Thanks for the discussion and review! Coleen David ----- 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

More information about the hotspot-dev mailing list