RFR(XS): JDK-8214272: Don't use memset to initialize arrays of MemoryUsage in memoryManager.cpp

[Roman Kennke]

Am 27. November 2018 16:54:34 MEZ schrieb Dmitry Chuyko <dmitry.chuyko at bell-sw.com>:
>On 11/27/18 6:30 PM, Roman Kennke wrote:
>> Good!
>>
>> I'm also getting similar warnings in:
>>
>> output.cpp:1707
>Thanks, forgot about this one. I created JDK-8214376 for that, it makes
>
>sense to post isolated changes even they seem trivial.
>> zForwardingTable.cpp:42
>
>My gcc is 8.2.0 and I don't get error here or I fail earlier. And you 
>probably have 8.2.1. I think it also makes sense to make a separate
>RFE.

Sure. You might also have zgc disabled (it is by default).

Roman

>-Dmitry
>
>>
>> Roman
>>
>>> The hint is from Kim Barrett.
>>>
>>> Surprisingly exactly this kind of warning is not silent just in 2
>>> places. I added matcher.cpp and re-submitted dev-submit tests.
>>>
>>> webrev: http://cr.openjdk.java.net/~dchuyko/8214272/webrev.01/
>>>
>>> -Dmitry
>>>
>>> On 11/27/18 5:39 PM, Roman Kennke wrote:
>>>> Oh. Right. There is always something to learn about C++ :-) Your
>patch
>>>> looks good then. And actually, may want to fix 'my' change too, if
>you
>>>> feel like it. And I believe gcc8 prints more similar warnings.
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>>>
>>>>> Doesn't "= MemoryUsage()" actually imply "delete onstack
>MemoryUsage"
>>>>> after copying? Probably safe here but it may be not safe in
>general.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 11/27/18 5:14 PM, Roman Kennke wrote:
>>>>>> I think Aleksey's version is clearer. I used a similar approach
>in a
>>>>>> fix
>>>>>> for the same issue:
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk/jdk/rev/710e5a66a64e
>>>>>>
>>>>>> Roman
>>>>>>
>>>>>>
>>>>>>> Placement new sets values placed in arrays. The arrays as a
>whole are
>>>>>>> managed as before. I.e.:
>>>>>>>
>>>>>>> GCStatInfo::~GCStatInfo() {
>>>>>>>      FREE_C_HEAP_ARRAY(MemoryUsage*, _before_gc_usage_array);
>>>>>>>      FREE_C_HEAP_ARRAY(MemoryUsage*, _after_gc_usage_array);
>>>>>>> }
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>> On 11/27/18 5:00 PM, Aleksey Shipilev wrote:
>>>>>>>> On 11/27/18 2:51 PM, Dmitry Chuyko wrote:
>>>>>>>>> rfe: https://bugs.openjdk.java.net/browse/JDK-8214272
>>>>>>>>> webrev: http://cr.openjdk.java.net/~dchuyko/8214272/webrev.00/
>>>>>>>> Ummmm. Aren't you leaking memory by calling "new" without
>"delete"?
>>>>>>>>
>>>>>>>> Consider this instead:
>>>>>>>>
>>>>>>>> diff -r a554db76b2e9
>src/hotspot/share/services/memoryManager.cpp
>>>>>>>> --- a/src/hotspot/share/services/memoryManager.cpp      Fri Nov
>23
>>>>>>>> 11:22:31 2018 +0100
>>>>>>>> +++ b/src/hotspot/share/services/memoryManager.cpp      Tue Nov
>27
>>>>>>>> 14:56:09 2018 +0100
>>>>>>>> @@ -169,7 +169,8 @@
>>>>>>>>        _start_time = 0L;
>>>>>>>>        _end_time = 0L;
>>>>>>>> -  size_t len = _usage_array_size * sizeof(MemoryUsage);
>>>>>>>> -  memset(_before_gc_usage_array, 0, len);
>>>>>>>> -  memset(_after_gc_usage_array, 0, len);
>>>>>>>> +  for (int i = 0; i < _usage_array_size; i++) {
>>>>>>>> +    _before_gc_usage_array[i] = MemoryUsage();
>>>>>>>> +    _after_gc_usage_array[i]  = MemoryUsage();
>>>>>>>> +  }
>>>>>>>>      }
>>>>>>>>
>>>>>>>> See, for example:
>>>>>>>>      https://bugs.openjdk.java.net/browse/JDK-8213745
>>>>>>>>
>>>>>>>> -Aleksey
>>>>>>>>