RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

[Doerr, Martin]

Hi Mikael,

I found out how the oop result processing works on x86. I was kind of confused about it, yesterday.
The oop case just has an additional piece of code which forces the oop to be alive and visible to GC by copying it from the (possibly weak) handle to the oop result field. Later, the result handler reloads it from there.

The current PPC64 implementation doesn't need the additional piece of code, because the oop lives in the (possibly weak) handle until the result handler loads it from there.

We need to decide if we keep the PPC64 implementation or not. Not sure if it's worth porting this diff to other platforms.

Best regards,
Martin


-----Original Message-----
From: Mikael Gerdin [mailto:mikael.gerdin at oracle.com] 
Sent: Dienstag, 24. Januar 2017 15:50
To: Doerr, Martin <martin.doerr at sap.com>; Kim Barrett <kim.barrett at oracle.com>
Cc: s390x-port-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net; hotspot-dev developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR: 8166188: G1 Needs pre barrier on dereference of weak JNI handles

Hi Martin,

Thanks for taking a look at this change.

On 2017-01-23 19:46, Doerr, Martin wrote:
> Hi everybody,
>
> thanks for addressing this issue and for the webrev.
>
> While reading code, I was wondering if the result handler for oop types ever gets called (except on PPC64).
> Looks like it is only there to compare the pointer after the native call and do something special in this case instead of calling it.
>
> Wouldn't it be cleaner to use the result handler and take care of the G1 pre barrier there?

Can't say I know much about the result handlers in general but I get the feeling that they are only there for the interpreter.
Are the result handlers applied to the return values of compiled native wrappers as well?

Thanks
/Mikael

>
> Btw. PPC64 currently uses a different implementation which really calls the oop result handler. The native entry saves the handle to the frame's lresult field and sets the oop_temp to NULL. The oop result handler takes care of the unboxing. In addition frame::interpreter_frame_result should use JNIHandles::resolve to retrieve the oop (which is not yet implemented as it should be). Also, one has to make sure that the active handles don't get reset too early.
> This implementation doesn't force the oop to be live. GC may still set it to NULL if it decides to kill the jweak.
>
> I think PPC64 and the other platforms should do it the same way. The question is what's the better approach.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: s390x-port-dev [mailto:s390x-port-dev-bounces at openjdk.java.net] 
> On Behalf Of Kim Barrett
> Sent: Donnerstag, 19. Januar 2017 19:05
> To: Mikael Gerdin <mikael.gerdin at oracle.com>
> Cc: s390x-port-dev at openjdk.java.net; 
> ppc-aix-port-dev at openjdk.java.net; hotspot-dev developers 
> <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR: 8166188: G1 Needs pre barrier on dereference of weak 
> JNI handles
>
>> On Jan 19, 2017, at 11:28 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 2017-01-19 06:31, Kim Barrett wrote:
>>> Please review this change to jweak to support G1.
>>>
>>> [...]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8166188/hotspot.02/
>>
>> I thought a bit about this issue today and unfortunately I've uncovered yet another location where a weak JNI handle needs to have its weak tag cleared.
>> Consider the following JNI code snippet
>>
>> [...]
>> I've created a test to provoke this situation and put it at:
>> http://cr.openjdk.java.net/~mgerdin/8166188-pass-arguments/webrev/
>> run the test with
>> $ make test-hotspot-jtreg-native
>>
>> Sorry for not coming up with this sooner, I just realized this problem today.
>
> Well, I'm happy you found it before this went out the door.  Despite looking for this sort of thing, I completely missed this one.  I need to re-review casts to oop*.
>