(10) RFR (actually S) 8169881: Remove implicit Handle conversions oop->Handle
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Feb 15 07:18:30 UTC 2017
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?
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.
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.
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.
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