RFR (S): 8022494: Make compilation IDs sequential

[Vladimir Kozlov]

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