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

[Vladimir Kozlov]

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
>