RFR (S): 8022494: Make compilation IDs sequential
Christian Thalinger
christian.thalinger at oracle.com
Thu Jan 9 10:56:35 PST 2014
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