RFR: 6543126: Level.known can leak memory

[Peter Levart]

Hi Daniel,


This is not strictly necessary, but here are a couple of things you 
might consider doing while changing this code:


Using new JDK 8 Map API, the following could be simplified:

  580         static synchronized void add(Level l) {
  581             purge();
  582             // the mirroredLevel object is always added to the list
  583             // before the custom Level instance
  584             KnownLevel o = new KnownLevel(l);
  585             List<KnownLevel> list = nameToLevels.get(l.name);
  586             if (list == null) {
  587                 list = new ArrayList<>();
  588                 nameToLevels.put(l.name, list);
  589             }
  590             list.add(o);
  591
  592             list = intToLevels.get(l.value);
  593             if (list == null) {
  594                 list = new ArrayList<>();
  595                 intToLevels.put(l.value, list);
  596             }
  597             list.add(o);
  598         }


into...

static synchronized void add(Level l) {
     purge();
     KnownLevel o = new KnownLevel(l);
     nameToLevels.computeIfAbsent(l.name, n -> new ArrayList<>()).add(o);
     intToLevels.computeIfAbsent(l.value, v -> new ArrayList<>()).add(o);
}


Removing a KnownLevel could also remove ArrayList(s) when they become empty:

  561         private void remove() {
  562 
Optional.ofNullable(nameToLevels.get(mirroredLevel.name)).ifPresent((x) 
-> x.remove(this));
  563 
Optional.ofNullable(intToLevels.get(mirroredLevel.value)).ifPresent((x) 
-> x.remove(this));
  564         }


...

private void remove() {
     nameToLevels.computeIfPresent(mirroredLevel.name, (n, l) -> 
l.remove(this) && l.isEmpty() ? null : l);
     intToLevels.computeIfPresent(mirroredLevel.value, (v, l) -> 
l.remove(this) && l.isEmpty() ? null : l);
}



Regards, Peter

And if you implement Mandy's suggestion to return Optional<Level>
On 08/10/2016 06:21 PM, Mandy Chung wrote:
>> On Jul 29, 2016, at 4:54 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
>>
>> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.01/
> This looks pretty good.
>
> Since KnownLevel is now a Reference, I suggest to change KnownLevel::findByName, findByValue and findByLocalizedLevelName to return Optional<Level> instead such that the parse method implementaiton could be simplified.
>
> Mandy