(10) RFR (actually S) 8169881: Remove implicit Handle conversions oop->Handle
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Feb 16 03:07:22 UTC 2017
On 2/15/17 17:46, coleen.phillimore at oracle.com wrote:
>
>
> 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();
> }
Yes, of course.
Sorry, I did not pay my attention to this detail.
I hope, you have caught it quickly.
Thanks,
Serguei
>
> 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