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

[Tobias Hartmann]

Hi,

thanks for the review and sorry for the delay.

Should I try to implement and test the suggested changes or wait for 
someone from runtime to have a look at it?

Thanks,
Tobias

On 16.07.2014 20:56, Vladimir Kozlov wrote:
> Lets see how big changes you need.
>
> Vladimir K
>
> On 7/16/14 10:49 AM, 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