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

[Vladimir Kozlov]

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