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

[Tobias Hartmann]

Thanks, David.

Best,
Tobias

On 09.09.2014 19:43, David Chase wrote:
> I am not a Reviewer, but it looks good to me.
>
> David
>
> On 2014-09-09, at 12:58 PM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
>
>> Thank you, Vladimir.
>>
>> Can I get another review?
>>
>> Best,
>> Tobias
>>
>> On 09.09.2014 17:45, Vladimir Kozlov wrote:
>>> 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