Thank you, Vladimir.
Best,
Albert
On 01/07/2014 09:02 AM, Vladimir Kozlov wrote:
> It is good.
>
> Thanks,
> Vladimir
>
> On 1/6/14 11:23 PM, Albert Noll 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.
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>