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

[Tobias Hartmann]

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