RFR [XS]: 8233013: deallocate memory after using NEW_C_HEAP_ARRAY

[David Holmes]

Hi Kim,

On 28/10/2019 1:35 pm, Kim Barrett wrote:
>> On Oct 27, 2019, at 6:32 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 26/10/2019 10:57 am, Kim Barrett wrote:
>>>> On Oct 25, 2019, at 9:46 AM, Baesken, Matthias <matthias.baesken at sap.com> wrote:
>>>>
>>>> Hello, please review this small  change . It adds  some missing  FREE_C_HEAP_ARRAY  calls  to os::init_system_properties_values   .
>>>>
>>>> bug/webrev :
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8233013
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8233013.1/
>>> I don’t think any of the proposed additional FREE_C_HEAP_ARRAY uses are needed.
>>
>> Not essential but also not harmful. See below …
> 
> I don't agree that it's harmless.  See below ...
> 
>>>> (but in case you think we do not need the freeing because of the calls are followed  by   " vm_exit_during_initialization"   calls,
>>>> then probably the already in-place freeing before those calls  could be removed  too  for consistency see
>>>>
>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8233013.1/src/hotspot/os/solaris/os_solaris.cpp.frames.html
>>>>
>>>> lines 456 / 466)
>>> I don’t think those are needed either.
>>
>> These stem from a time when there was still the view that vm_exit_during_initialization could/should unload the VM rather than blow away the hosting process (which may not be the launcher). I think that is a battle we have abandoned now.
> 
> I agree; that ship has sailed.
> 
>> I think this is going to be messy of we try to fight the consistency battle in this area. But I can see an argument for putting in the free call if we wanted to allow the code to be more easily examined by code checkers that were looking at correct usage here. So I don't object to this cleanup.
> 
> I don't think these help any actually useful code checkers (e.g. ones
> which look at more than a single translation unit), which will see
> that the code path leads to abort or exit.

It requires that you have to teach the tool about 
vm_exit_during_initialization. I don't know if that is a hurdle or not 
but it seems an unnecessary complexity to have to add.

> I really dislike adding effectively useless code.  It expands the code
> surface area for no good reason.  That has various knock-on effects.
> For example, some of these lines have been modified multiple times
> over the years as our memory management infrastructure has changed.

I'm not sure I see your point re "knock-on effects".

> If we really want consistency, I'd remove the solaris-specific useless
> code. But because of JEP 362, that doesn't seem to me to be worth the
> effort. Of course, we've probably now spent more effort...
> 
> The best way to handle this sort of thing would be to capture the
> temporary buf in an RAII object at the point of construction. That's
> true in many other places too.

So if we had been using RAII then this somehow makes the freeing code 
not useless? The syntax shouldn't make a difference.

I'm fine either way with this change. I still see it as unnecessary but 
harmless (by my own definition of "harm").

Cheers,
David