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

[Dmitry Chuyko]

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.

-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
>>>>>>>