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

[Vladimir Ivanov]

Any thoughts about that? I still think "_entry_bci = InvalidOSREntryBci" 
is redundant.

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