PING: RFR: JDK-8153743: AllocateHeap() and ReallocateHeap() should use ALWAYSINLINE macro

Wed May 11 14:04:55 UTC 2016

Hi,

I've sent review request for ALWAYSINLINE macro.
Could you review it?

>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.01/

I need a second reviewer.

Thanks,

Yasumasa

On 2016/05/05 11:21, Yasumasa Suenaga wrote:
> Hi David,
>
>> It did pass jprt.
>
> Thanks!
>
>> But this suggests to me our inlining attempts may not actually be working as we expect.
>
> I think it is inlined.
> I run nm command for fastdebug libjvm on Linux, I cannot found symbols in instanceKlass.inline.hpp .
> (I do not have Solaris, so I cannot confirm it on Solaris.)
>
>
>> A second review of this is still needed anyway, so I've added Kim to the cc list for his compiler expertise.
>
> Okay, I'm waiting second reviewer.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/05/05 11:09, David Holmes wrote:
>> On 4/05/2016 11:17 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>>> We also require:
>>>>
>>>> diff -r fc5b64f70199
>>>> src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>>
>>> Thanks!
>>> I added it to new webrev:
>>>
>>>   http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.01/
>>>
>>>>> I'm running it through our build system.
>>>
>>> If this patch passes JPRT, I think this patch is ready to merge.
>>
>> It did pass jprt. I am wondering though, why the existing uses of ALWAYSINLINE in instanceKLass.inline.hpp compile fine on Solaris without needing the addition of "inline" to the ALWAYSINLINE macro? All those uses appear in template definitions so perhaps that influences things somehow? But this suggests to me our inlining attempts may not actually be working as we expect.
>>
>> A second review of this is still needed anyway, so I've added Kim to the cc list for his compiler expertise.
>>
>> Thanks,
>> David
>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/05/04 19:16, David Holmes wrote:
>>>> On 4/05/2016 12:10 PM, David Holmes wrote:
>>>>> On 25/04/2016 10:11 PM, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Could you review and sponsor it?
>>>>>> I confirmed this patch can apply to jdk9/hs repos.
>>>>>>
>>>>>> However, I checked this patch with GCC on Linux.
>>>>>> I cannot check with other compiler.
>>>>>
>>>>> I'm running it through our build system.
>>>>
>>>> We also require:
>>>>
>>>> diff -r fc5b64f70199
>>>> src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>>> --- a/src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>>> +++ b/src/share/vm/utilities/globalDefinitions_sparcWorks.hpp
>>>> @@ -279,6 +279,6 @@
>>>>
>>>>  // Inlining support
>>>>  #define NOINLINE
>>>> -#define ALWAYSINLINE __attribute__((always_inline))
>>>> +#define ALWAYSINLINE inline __attribute__((always_inline))
>>>>
>>>>  #endif // SHARE_VM_UTILITIES_GLOBALDEFINITIONS_SPARCWORKS_HPP
>>>>
>>>> David
>>>>
>>>>
>>>>>> Also other compilers might need "inline" statement in ALWAYSINLINE
>>>>>> macro.
>>>>>> So I want to hear whether this patch can work with other compilers.
>>>>>
>>>>> I don't have any input on this - sorry.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2016/04/16 1:10, Daniel D. Daugherty wrote:
>>>>>>> JDK9-hs and JDK9-hs-rt are currently locked down due to the merge.
>>>>>>> Please see the "Merging jdk9/hs-rt with jdk9/hs" e-mail thread on
>>>>>>> the hotspot-dev at openjdk.java.net alias.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4/15/16 7:28 AM, Yasumasa Suenaga wrote:
>>>>>>>> PING: Could you review and sponsor it?
>>>>>>>>
>>>>>>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.00/
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2016/04/07 23:13, Yasumasa Suenaga wrote:
>>>>>>>>> For force inlining, JDK-8076212 uses always_inline attribute to
>>>>>>>>> them.
>>>>>>>>> JDK-8151593 added ALWAYSINLINE macro for force inlining.
>>>>>>>>>
>>>>>>>>> For consistency, and for other compiler support, AllocateHeap() and
>>>>>>>>> ReallocateHeap() should use ALWAYSINLINE.
>>>>>>>>>
>>>>>>>>> I used ALWAYSINLINE to them, but I got error message as below;
>>>>>>>>> ------------------------------
>>>>>>>>> Building target 'images' in configuration
>>>>>>>>> 'linux-x86_64-normal-server-release'
>>>>>>>>> In file included from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/utilities/array.hpp:29:0,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                   from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/memory/universe.hpp:29,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                   from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/code/oopRecorder.hpp:28,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                   from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/asm/codeBuffer.hpp:28,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                   from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/asm/assembler.hpp:28,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                   from
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/precompiled/precompiled.hpp:29:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> /home/ysuenaga/OpenJDK/hs-rt/hotspot/src/share/vm/memory/allocation.inline.hpp:72:20:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> エラー: always_inline function might not be inlinable
>>>>>>>>> [-Werror=attributes]
>>>>>>>>>   ALWAYSINLINE char* ReallocateHeap(char *old, size_t size, MEMFLAGS
>>>>>>>>> flag,
>>>>>>>>>                      ^
>>>>>>>>> ------------------------------
>>>>>>>>> * This message includes Japanese because my environment is Japanese
>>>>>>>>> locale :-)
>>>>>>>>>
>>>>>>>>> According to GCC manual [1], non-static inline function is always
>>>>>>>>> compiled
>>>>>>>>> on its own in the usual fashion.
>>>>>>>>> However, we can compile as inline function with "inline" and
>>>>>>>>> "always_inline".
>>>>>>>>> always_inline attribute does not imply inlining [2].
>>>>>>>>> GCC testcase [3] uses both "inline" and "always_inline" actually.
>>>>>>>>>
>>>>>>>>> I uploaded webrev. Could you review it?
>>>>>>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8153743/webrev.00/
>>>>>>>>>
>>>>>>>>> I cannot access JPRT.
>>>>>>>>> So I need a sponsor.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Inline.html
>>>>>>>>> [2] https://chromiumcodereview.appspot.com/14820003/
>>>>>>>>> [3]
>>>>>>>>> https://gcc.gnu.org/viewcvs/gcc/trunk/gcc/testsuite/gcc.dg/vmx/gcc-bug-i.c?view=markup&pathrev=178730
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>