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

[Baesken, Matthias]

Hi David / Kim ,  thanks for your comments.

 I think I just document  the  outcome of   this little discussion  in https://bugs.openjdk.java.net/browse/JDK-8233013 
  and then close the item .

Best regards, Matthias


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