[9] RFR(S): 8048721: -XX:+PrintCompilation prints negative bci for non entrant OSR methods

Tue Sep 9 17:43:27 UTC 2014

I am not a Reviewer, but it looks good to me.

David

On 2014-09-09, at 12:58 PM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:

> Thank you, Vladimir.
> 
> Can I get another review?
> 
> Best,
> Tobias
> 
> On 09.09.2014 17:45, Vladimir Kozlov wrote:
>> Looks good.
>> 
>> Thanks,
>> Vladimir
>> 
>> On 9/9/14 12:46 AM, Tobias Hartmann wrote:
>>> Hi Vladimir,
>>> 
>>> thanks for the review. I'm now using 'nmethod::in_use' instead of 0.
>>> 
>>> New webrev: http://cr.openjdk.java.net/~thartmann/8048721/webrev.02/
>>> 
>>> Thanks,
>>> Tobias
>>> 
>>> On 08.09.2014 19:04, Vladimir Kozlov wrote:
>>>> Tobias,
>>>> 
>>>> Please, use nmethod::in_use instead of 0 in cmp instructions. You need to make the enum public.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 9/8/14 6:50 AM, Tobias Hartmann wrote:
>>>>> Hi,
>>>>> 
>>>>> sorry for the delay. As suggested, I completely removed the setting of 'InvalidOSREntryBci'. To prevent a non-entrant
>>>>> OSR nmethod from being used again, I'm now checking the value of 'nmethod::_state' instead. As Vladimir I. mentioned
>>>>> there is no synchronization and therefore it could still happen that an old value of 'nmethod::_state' is observed and a
>>>>> non-entrant OSR method is invoked. In my opinion this is not a problem because invoking the OSR method will only trigger
>>>>> deoptimization again and the updated state-value will eventually be observed.
>>>>> 
>>>>> Because nmethod::_state is a byte variable I also adapted the assembly code for loading and comparing the value.
>>>>> 
>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8048721/webrev.01/
>>>>> 
>>>>> I tested the implementation with VMSQE.adhoc.JPRT and -XX:+DeoptimizeALot.
>>>>> 
>>>>> Thanks,
>>>>> Tobias
>>>>> 
>>>>> On 16.07.2014 19:49, Vladimir Ivanov wrote:
>>>>>> Thanks, Vladimir.
>>>>>> 
>>>>>> On 7/11/14 2:13 AM, Vladimir Kozlov wrote:
>>>>>>> _method set to NULL also in make_unloaded() but after
>>>>>>> invalidate_osr_method() call so it should be fine.
>>>>>>> 
>>>>>>> On 7/8/14 5:51 AM, Vladimir Ivanov wrote:
>>>>>>>> Any thoughts about that? I still think "_entry_bci = InvalidOSREntryBci"
>>>>>>>> is redundant.
>>>>>>> 
>>>>>>> Unfortunately profiling code in Interpeter test this value:
>>>>>>> 
>>>>>>>       __ movl(rcx, Address(rax, nmethod::entry_bci_offset()));
>>>>>>>       __ cmpl(rcx, InvalidOSREntryBci);
>>>>>>> 
>>>>>>> So it would require a lot more changes. But I agree that changing
>>>>>>> _entry_bci looks strange
>>>>>>> 
>>>>>>> Can we check _state value instead of _entry_bci? Could be some race
>>>>>>> conditions why _entry_bci is used.
>>>>>> I don't think it's necessary.
>>>>>> 
>>>>>> Since both lookup (InstanceKlass::lookup_osr_nmethod) and removal
>>>>>> (InstanceKlass::remove_osr_nmethod) actions synchronize on OsrList_lock, the only case when _entry_bci !=
>>>>>> InvalidOSREntryBci matters is the following:
>>>>>> - T1: Interpreter fetches an OSR nmethod
>>>>>> - T2: invalidates OSR nmethod and writes InvalidOSREntryBci into _entry_bci
>>>>>> - T1: checks "_entry_bci != InvalidOSREntryBci"
>>>>>> 
>>>>>> But it's already broken since it's not guaranteed that the value written by T2 will be seen by T1 (no synchronization
>>>>>> actions between T1 & T2).
>>>>>> 
>>>>>> I'd stop writing InvalidOSREntryBci into _entry_bci and remove "_entry_bci != InvalidOSREntryBci" check from
>>>>>> Interpreter, or leave it for further cleanup if it's too much for now. It's an innocent check.
>>>>>> 
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>> 
>>>>>>> 
>>>>>>> Vladimir K
>>>>>>> 
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>>>>>> 
>>>>>>>> On 7/1/14 4:02 PM, Vladimir Ivanov wrote:
>>>>>>>>> (widening audience to Runtime folks)
>>>>>>>>> 
>>>>>>>>> I don't think it's good to lose bci for OSR methods after nmethod
>>>>>>>>> invalidation.
>>>>>>>>> 
>>>>>>>>> Regarding current invalidation logic, I don't see that "_entry_bci =
>>>>>>>>> InvalidOSREntryBci" is required to invalidate OSR nmethods. Yes,
>>>>>>>>> interpreter does "_entry_bci != InvalidOSREntryBci" check before calling
>>>>>>>>> OSR nmethod, but _entry_bci update isn't properly synchronized between
>>>>>>>>> invalidating and querying threads (no locks or memory barriers
>>>>>>>>> involved).
>>>>>>>>> 
>>>>>>>>> So I have a question is it safe to remove "_entry_bci =
>>>>>>>>> InvalidOSREntryBci".
>>>>>>>>> 
>>>>>>>>> Both OSR nmethod lookup (InstanceKlass::lookup_osr_nmethod) and removal
>>>>>>>>> (InstanceKlass::remove_osr_nmethod) synchronize on OsrList_lock, so
>>>>>>>>> after removal OSR nmethod can't be looked up.
>>>>>>>>> 
>>>>>>>>> What bothers me is the following null check before removal:
>>>>>>>>> void nmethod::invalidate_osr_method() {
>>>>>>>>>   if (method() != NULL)
>>>>>>>>> method()->method_holder()->remove_osr_nmethod(this);
>>>>>>>>> 
>>>>>>>>> Under what conditions nmethod with _method == NULL can be invalidated?
>>>>>>>>> Or is it just defensive coding?
>>>>>>>>> 
>>>>>>>>> We set _method to NULL only for zombie nmethods (see
>>>>>>>>> nmethod::make_not_entrant_or_zombie) and they are already invalidated at
>>>>>>>>> that point.
>>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>>>>>> 
>>>>>>>>> On 7/1/14 2:48 PM, Tobias Hartmann wrote:
>>>>>>>>>> Hi,
>>>>>>>>>> 
>>>>>>>>>> please review the following patch for JDK-8048721.
>>>>>>>>>> 
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8048721
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8048721/webrev.00/
>>>>>>>>>> 
>>>>>>>>>> *Problem*
>>>>>>>>>> The VM option -XX:+PrintCompilation prints a negative bytecode index
>>>>>>>>>> for
>>>>>>>>>> not entrant OSR nmethods. The problem is that in
>>>>>>>>>> nmethod::make_not_entrant_or_zombie(..) the _entry_bci is invalidated
>>>>>>>>>> (i.e. set to InvalidOSREntryBci == -2) by invalidate_osr_method(..) and
>>>>>>>>>> log_state_change(..) is called afterwards, printing the invalidated
>>>>>>>>>> entry.
>>>>>>>>>> 
>>>>>>>>>> *Solution
>>>>>>>>>> *Because reordering the methods in
>>>>>>>>>> nmethod::make_not_entrant_or_zombie(..) is not easily possible due to
>>>>>>>>>> thread safety, I changed the implementation log_state_change(..) and
>>>>>>>>>> dependent methods to take the entry bci as parameter. The entry bci is
>>>>>>>>>> saved prior to invalidation and used for printing afterwards.
>>>>>>>>>> 
>>>>>>>>>> *Testing*
>>>>>>>>>> JPRT
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> Tobias
>>>>> 
>>> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140909/2e323f64/signature.asc>