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

[Tobias Hartmann]

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