(10) RFR (actually S) 8169881: Remove implicit Handle conversions oop->Handle
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* cur_second = NULL;
>> - Handle object = values->obj_at(i);
>> - BasicType type =
>> JVMCIRuntime::kindToBasicType(slotKinds->obj_at(i), CHECK);
>> + Handle object (THREAD, values->obj_at(i));
>> + Handle slot_kind (THREAD, slotKinds->obj_at(i));
>> + BasicType type = JVMCIRuntime::kindToBasicType(slot_kind, 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.is_null()) {
>> - result = CompilerToVM::get_jvmci_type(klass, CHECK_NULL);
>> + oop result_oop = CompilerToVM::get_jvmci_type(klass, CHECK_NULL);
>> + result = Handle(THREAD, result_oop);
>> } else {
>> result = java_lang_String::create_from_symbol(symbol, CHECK_NULL);
>> }
>> return JNIHandles::make_local(THREAD, result());
>>
>>
>> Not clear, why does the result need to be a handle.
>>
>
> There's no reason this had to be a Handle, but
> java_lang_String::create_from_symbol returns a Handle. That's why.
> This seems better, agree?
>
> oop result_oop;
> if (!klass.is_null()) {
> oop result_oop = CompilerToVM::get_jvmci_type(klass, CHECK_NULL);
> } else {
> Handle result = java_lang_String::create_from_symbol(symbol,
> CHECK_NULL);
> result_oop = result();
> }
> return JNIHandles::make_local(THREAD, result_oop);
>
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(class_name->as_C_string(), JAVAPKG, JAVAPKG_LEN) == 0) {
>> + strncmp(class_name->as_C_string(), 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 (!class_loader.is_null() &&
> - !SystemDictionary::is_platform_class_loader(class_loader) &&
> + !SystemDictionary::is_platform_class_loader(class_loader()) &&
> class_name != NULL &&
> strncmp(class_name->as_C_string(), JAVAPKG, JAVAPKG_LEN) == 0) {
> TempNewSymbol pkg_name =
> InstanceKlass::package_from_name(class_name, CHECK);
>
>>
>>
>> http://cr.openjdk.java.net/~coleenp/8169881.02/webrev/src/share/vm/oops/instanceKlass.cpp.frames.html
>>
>>
>> 701 void InstanceKlass::initialize_impl(instanceKlassHandle this_k,
>> 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::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.
>>>>
>>>> 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 obj_field, etc.
>>>
>>> oop java_lang_Throwable::message(oop throwable) {
>>> return throwable->obj_field(detailMessage_offset);
>>> }
>>>
>>> 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 = java_lang_Throwable::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 = preallocated_out_of_memory_errors()->obj_at(next);
>>> assert(exc != NULL, "slot has been used already");
>>> preallocated_out_of_memory_errors()->obj_at_put(next, NULL);
>>> <= This on G1 can take out a lock for the oop_store and GC and move
>>> the default_err
>>>
>>> // use the message from the default error
>>> oop msg = java_lang_Throwable::message(default_err); <=
>>> default_err is an oop.
>>> assert(msg != NULL, "no message");
>>> java_lang_Throwable::set_message(exc, msg);
>>>
>>> exc is a naked oop too. I don't think the naked oop detector
>>> triggers for obj_at_put/oop_store 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->_handle_area.
>>>>
>>>> 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/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.
>>>>
>>>> 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 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.
>>>>
>>>> I have to disagree with this one. AFAICS there is no reason this:
>>>>
>>>> 517 Symbol* java_lang_String::as_symbol(Handle java_string, TRAPS) {
>>>>
>>>> needs to take a Handle in the first place. The java_string 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 _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.
>>>>
>>>> Ok.
>>>>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> 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.
>>>>
>>>> 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