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

[Tobias Hartmann]

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
>>>>
>>