(10) RFR (actually S) 8169881: Remove implicit Handle conversions oop->Handle
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Feb 14 23:16:50 UTC 2017
On 2/14/17 6:03 PM, David Holmes wrote:
> Hi Coleen,
>
> On 15/02/2017 8:21 AM, coleen.phillimore at oracle.com wrote:
>>
>> New webrev:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8169881.02/webrev
>
> Thanks for bearing with me on this. Changes seem fine - thanks for
> updating javaClasses to not use Handles unnecessarily.
Thanks for being brave enough to look at this change!
>
> Nits: src/share/vm/jvmci/jvmciCodeInstaller.cpp
>
> There are quite a few handle declarations with a space between the
> variable and constructor ( eg:
>
> Handle location (THREAD, objects->obj_at(i));
>
> instead of
>
> Handle location(THREAD, objects->obj_at(i));
>
> There are over 40 in that file (I'm guessing you had a semi-automated
> conversion mechanism).
>
Nah, I just got tired of typing. I'll fix them.
> ---
>
> Nit: src/share/vm/runtime/handles.hpp
>
> -// Handle h1(obj); // allocate new handle
> // Handle h2(thread, obj); // faster allocation when current
> thread is known
> // Handle h3; // declare handle only, no
> allocation occurs
>
> the remaining comments don't quite make sense with the deletion of the
> first line. Perhaps just:
>
> // Handle h1(thread, obj); // allocate new handle in thread
> // Handle h2; // declare handle only, no
> allocation occurs
>
ok, I'll change as suggested.
> Thanks. No need to see updated webrev if you do update.
>
Thanks!
Coleen
> David
> -----
>
>> 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