RFR (S): 8022494: Make compilation IDs sequential

[Christian Thalinger]

One thing we might want to add a comment for is that in a product build we only increase _compilation_id  and not _osr_compilation_id.  This is fine because CICountOSR is a develop flag with a default value of false but it is confusing to the reader.

+     id = Atomic::add(1, &_osr_2compilation_id);

Typo.

Otherwise this looks good.

On Jan 6, 2014, at 11:23 PM, Albert Noll <albert.noll at oracle.com> wrote:

> Hi Vladimir,
> 
> sorry, I misunderstood your suggestion, Now it makes sense.
> Here is the new webrev that contains your proposed solution.
> 
> http://cr.openjdk.java.net/~anoll/8022494/webrev.06/
> 
> Best,
> Albert
> 
> On 01/06/2014 11:36 PM, Vladimir Kozlov wrote:
>> Albert,
>> 
>> Next comment does not sound correct:
>> 
>> ! // These counters are used to assign each compilation an unique ID
>> 
>> I think the original was more correct with small correction:
>> 
>> ! // These counters are used to assign an unique ID to each compilation
>> 
>> 
>> And you did not fix it as I asked:
>> 
>> >> I suggested to generate compile_id always in such case and convert
>> >> your warning to assert (since it could only happens in debug VM).
>> 
>> I suggested next:
>> 
>> int CompileBroker::assign_compile_id(methodHandle method, int osr_bci) {
>> #ifdef ASSERT
>>  bool is_osr = (osr_bci != standard_entry_bci);
>>  int id;
>>  if (method->is_native()) {
>>    assert(!is_osr, "can't be osr");
>>    // Adapters, native wrappers and method handle intrinsics
>>    // should be generated always.
>>    return Atomic::add(1, &_compilation_id);
>>  } else if (CICountOSR && is_osr) {
>>    id = Atomic::add(1, &_osr_compilation_id);
>>    if (CIStartOSR <= id && id < CIStopOSR) {
>>      return id;
>>    }
>>  } else {
>>    id = Atomic::add(1, &_compilation_id);
>>    if (CIStart <= id && id < CIStop) {
>>      return id;
>>    }
>>  }
>> 
>>  // Method was not in the appropriate compilation range.
>>  method->set_not_compilable_quietly();
>>  return 0;
>> #else
>>  return Atomic::add(1, &_compilation_id);
>> #endif
>> }
>> 
>> The assert should stay in create_native_wrapper() as in your previous version:
>> 
>> +     const int compile_id = CompileBroker::assign_compile_id(method, CompileBroker::standard_entry_bci);
>> +     assert(compile_id > 0, "Must generate native wrapper");
>> 
>> Thanks,
>> Vladimir
>> 
>> 
>> On 1/6/14 12:07 AM, Albert Noll wrote:
>>> Hi Vladimir,
>>> 
>>> thanks for your explanation. I agree with your suggestion. The new
>>> version has the
>>> corresponding check inside
>>> 
>>> *CompileBroker::assign_compile_id()*
>>> 
>>> >Also why you return only in such case and not for normal native wrappers?
>>> 
>>> Thanks for catching this bug. I fixed it in the new version.
>>> 
>>> 
>>> Here is the link to the new webrev:
>>> http://cr.openjdk.java.net/~anoll/8022494/webrev.05/
>>> 
>>> Best,
>>> Albert
>>> 
>>> 
>>> On 12/21/2013 08:46 PM, Vladimir Kozlov wrote:
>>>> We have to generate method_handle_intrinsic so we can't simple show
>>>> warning and continue execution - we can't do that.
>>>> I suggested to generate compile_id always in such case and convert
>>>> your warning to assert (since it could only happens in debug VM).
>>>> Also why you return only in such case and not for normal native wrappers?
>>>> 
>>>> +       if (method->is_method_handle_intrinsic()) {
>>>> +         warning("Must generate wrapper for method handle intrinsic");
>>>> +         return;
>>>> +       }
>>>> ---
>>>> +       assert(!method->is_method_handle_intrinsic()), "Must generate
>>>> wrapper for method handle intrinsic");
>>>> +       return;
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 12/18/13 10:18 PM, Albert Noll wrote:
>>>>> Christian, Vladimir, thanks for the review.
>>>>> 
>>>>> @Christian: Thanks for catching the typo
>>>>> 
>>>>> @Vladimir: I am not sure if I understand your suggestion correctly.
>>>>> Could you please clarify what you
>>>>> mean by "The warning above will be assert after that."
>>>>> 
>>>>> Best,
>>>>> Albert
>>>>> 
>>>>> 
>>>>> On 10/28/2013 07:59 PM, Vladimir Kozlov wrote:
>>>>>> Albert,
>>>>>> 
>>>>>> The warning is not correct solution since we HAVE to generate method
>>>>>> handle intrinsics if your comment is correct:
>>>>>> 
>>>>>> +       // must be generated for method handle intrinsics (8026407),
>>>>>> print out a warning.
>>>>>> +       if (method->is_method_handle_intrinsic()) {
>>>>>> +         warning("Must generate wrapper for method handle intrinsic");
>>>>>> +         return;
>>>>>> +       }
>>>>>> 
>>>>>> I think assign_compile_id() should generate id in such case
>>>>>> regardless CIStart and CIStop values. The warning above
>>>>>> will be assert after that.
>>>>>> 
>>>>>> And, please, file RFE (starter task) to cleanup type of compile_id.
>>>>>> In some places it declared as 'int' and in an
>>>>>> other as 'uint'.
>>>>>> 
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> On 10/24/13 1:56 AM, Albert Noll wrote:
>>>>>>> Here is the updated webrev:
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~anoll/8022494/webrev.04/
>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8022494/webrev.04/>
>>>>>>> 
>>>>>>> Best,
>>>>>>> Albert
>>>>>>> 
>>>>>>> On 24.10.2013 10:21, Albert Noll wrote:
>>>>>>>> Hi Aleksey,
>>>>>>>> 
>>>>>>>> thanks for looking at this.
>>>>>>>> 
>>>>>>>> On 24.10.2013 10:15, Aleksey Shipilev wrote:
>>>>>>>>> On 10/24/2013 12:01 PM, Albert Noll wrote:
>>>>>>>>>> Here is the updated webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~anoll/8022494/webrev.03/
>>>>>>>>> Nice to see the locking gone.
>>>>>>>>> 
>>>>>>>>> compileBroker.cpp:
>>>>>>>>>   * Is that considered correct that OSR and normal compilations are
>>>>>>>>> marked differently when running in debug mode, but not in release? I
>>>>>>>>> understand the comment before assign_compile_id, so this is more
>>>>>>>>> of the
>>>>>>>>> philosophical question.
>>>>>>>> Compilation IDs are only different if -XX:CICountOSR is set, which is
>>>>>>>> defaulted to false.
>>>>>>>>> sharedRuntime.cpp:
>>>>>>>>>   * Why do you need "2653   return;" in the method tail?
>>>>>>>> Thanks for spotting this. I missed it during the cleanup.
>>>>>>>> 
>>>>>>>> Best,
>>>>>>>> Albert
>>>>>>>>> Thanks,
>>>>>>>>> -Aleksey.
>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>