RFR (S): 8022494: Make compilation IDs sequential

[Albert Noll]

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