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

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Wed Jul 16 17:49:14 UTC 2014


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


More information about the hotspot-compiler-dev mailing list