RFR: 6543126: Level.known can leak memory

[Peter Levart]

Hi Daniel,

Another place that is not clear to me is the following (in old code):

  585         // Returns a KnownLevel with the given localized name matching
  586         // by calling the Level.getLocalizedLevelName() method (i.e. found
  587         // from the resourceBundle associated with the Level object).
  588         // This method does not call Level.getLocalizedName() that may
  589         // be overridden in a subclass implementation
590 static synchronized KnownLevel findByLocalizedLevelName(String name) {
591 for (List<KnownLevel> levels : nameToLevels.values()) {
592 for (KnownLevel l : levels) {
593 String lname = l.levelObject.getLocalizedLevelName();
594 if (name.equals(lname)) {
595 return l;
596 }
597 }
598 }
599 return null;
  600         }

In spite of claiming that "this method doesn't call 
Level.getLocalizedName() that may be overridden in subclass", it does 
just that. it calls the getLocalizedLevelName on the 
KnownLevel.levelObject, which is a user-constructed object. You changed 
the code into:

  627         static synchronized Optional<Level> 
findByLocalizedLevelName(String name,
  628                 Function<KnownLevel, Stream<Level>> selector) {
  629             purge();
  630             return nameToLevels.values()
  631                     .stream()
  632                     .flatMap(List::stream)
  633                     .flatMap(selector)
  634                     .filter(lo -> 
name.equals(lo.getLocalizedLevelName()))
  635                     .findFirst();
  636         }

Which calls getLocalizedName() on the Level object selected by the given 
selector. In line 385 you pass in the selector to select the 
mirroredLevel, but in line 487, you pass in the selector to select the 
referent.

So which one is the right one? If you want to obey the comment it should 
always be the mirroredLevel which is checked against localized name, right?

Regards, Peter


On 08/16/2016 12:42 PM, Daniel Fuchs wrote:
> Hi Mandy,
>
> I added an additional selector parameter to the find methods.
> This made it possible to return Optional<Level> instead of
> KnownLevel - and it does simply the parse() method.
>
> http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.02
>
> best regards,
>
> -- daniel
>
> On 11/08/16 20:12, Mandy Chung wrote:
>>
>>> On Aug 11, 2016, at 2:29 AM, Daniel Fuchs <daniel.fuchs at oracle.com> 
>>> wrote:
>>>
>>> On 10/08/16 17:21, 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.
>>>
>>> We need to return KnownLevel because sometimes we need the
>>> level object and sometimes the mirror.
>>
>> So either findByName(String name, boolean mirror) or two methods: 
>> findLevelByName and findMirroredLevelByName??
>>
>> Or seriously consider to remove KnownLevel class by introducing a new 
>> Level subclass with final Level.getName, Level.getLocalizedName, 
>> Level.getResourceBundleName methods??
>>
>> Mandy
>>
>