RFR (S): 8022494: Make compilation IDs sequential
Albert Noll
albert.noll at oracle.com
Thu Jan 9 06:50:01 PST 2014
Hi Chris,
thanks for looking at this again. I added a comment and fixed the typo.
Here is the new webrev:
http://cr.openjdk.java.net/~anoll/8022494/webrev.07/
Best,
Albert
On 01/08/2014 09:34 PM, Christian Thalinger wrote:
> 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.
More information about the hotspot-compiler-dev
mailing list