RFR (S): 8022494: Make compilation IDs sequential

[Vladimir Kozlov]

It is good.

Thanks,
Vladimir

On 1/6/14 11:23 PM, Albert Noll 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.
>>>>>>>>
>>>>>>>
>>>>>
>>>
>