RFR (S): 8022494: Make compilation IDs sequential
Albert Noll
albert.noll at oracle.com
Thu Jan 9 11:10:24 PST 2014
Vladimir, Chris, thanks for the reviews.
I will move the comment as suggested by Chris and push that version.
Best,
Albert
On 01/09/2014 07:56 PM, Christian Thalinger wrote:
> Looks good. Although I would’ve put the comment here:
>
> + #else
> + return Atomic::add(1, &_compilation_id);
> + #endif
>
> On Jan 9, 2014, at 6:50 AM, Albert Noll <albert.noll at oracle.com> wrote:
>
>> 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