RFR: 8188102: [JVMCI] Convert special JVMCI oops in nmethod to jweak values

[Tom Rodriguez]

Looks good to me.

tom

Doug Simon wrote:
>
>> On 2 Nov 2017, at 04:47, Kim Barrett<kim.barrett at oracle.com>  wrote:
>>
>>> On Nov 1, 2017, at 5:10 PM, Doug Simon<doug.simon at oracle.com>  wrote:
>>>
>>>
>>>
>>>> On 1 Nov 2017, at 18:57, Kim Barrett<kim.barrett at oracle.com>  wrote:
>>>>
>>>>> On Nov 1, 2017, at 5:44 AM, Erik Österlund<erik.osterlund at oracle.com>  wrote:
>>>>>
>>>>> Hi Kim,
>>>>>
>>>>> On 2017-11-01 06:03, Kim Barrett wrote:
>>>>>>> On Oct 30, 2017, at 7:14 AM, Doug Simon<doug.simon at oracle.com>  wrote:
>>>>> JNIHandles::resolve with G1 should conditionally enqueue the oops if the SATB queue is activated, which it will not be after marking terminated. At this point, marking should have terminated already, and hence the resolve will not change the liveness of the oop. So I don't quite see how the resolve will change the oop to live here. Am I missing something?
>>>> Does this nmethod pass get run during young collections?  If not, then maybe you are right that it doesn’t matter.
>>>>
>>>> But I still think that a cleanup pass that is based on using weak references like this should just be properly ordered
>>>> wrto weak reference processing, as it presumably already was.  In which case the resolve and can_unload is
>>>> effectively useless; either the jweak has been cleared and we won’t reach this clause, or the jweak has not been
>>>> cleared because the referent is still live, in which case can_unload will always return false and there was no point
>>>> in calling it.
>>> I agree. As stated previously, my (defensive) code is based on assuming that nmethod unloading might happen before reference processing. If it never/rarely happens, then your suggested change is the right one. I did some extra testing and never saw it happen. Based on that, I'd like to adopt your solution. In the worst case, nmethod unloading will be delayed one full GC cycle if nmethod unloading precedes reference processing.
>>>
>>> I've created another rev based on this:
>>>
>>> http://cr.openjdk.java.net/~dnsimon/8188102_3
>>>
>>> -Doug
>> This looks good.
>>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/runtime/jniHandles.cpp
>>
>> I'd like is_global_weak_cleared moved up a few lines, immediately
>> following the specializations of resolve_jweak, which I think it is
>> closely related to.
>>
>> I don't need another webrev for this.
>
> Will do. I updated the latest webrev in place.
>
>> ------------------------------------------------------------------------------
>> src/hotspot/share/code/nmethod.cpp
>>
>> 2934 char* nmethod::jvmci_installed_code_name(char* buf, size_t buflen) {
>> 2935   if (!this->is_compiled_by_jvmci()) {
>> 2936     return NULL;
>> 2937   }
>> 2938   oop installed_code = JNIHandles::resolve(_jvmci_installed_code);
>>
>> Are there any circumstances where it would be unfortunate to have
>> getting the name keep alive the object? I was thinking logging. Or
>> perhaps it's a feature that getting the name might artificially extend
>> the lifetime? Looking at the callers, it doesn't seem like a problem
>> right now.
>
> Right, this is only called from tracing or debugging code.
>
>> This is one of those places where a "peek" access (coming soon from
>> the GC interface) might be appropriate.
>
> This potential for resolving a jweak keeping the referent alive is unfortunate but I'm guessing it's also unavoidable.
>
> Thanks once again for a very helpful review!
>
> -Doug