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

[Vladimir Kozlov]

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

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