RFR: 6543126: Level.known can leak memory

[Daniel Fuchs]

Hi Peter,

>> Now that you have various findByXXX methods return Optional<Level>, you
>> could make methods that use them more Optional-API-powered. For example,
>> findLevel could be written as a single expression:

Right. But do you mind if I leave that for another day?
I'm not too keen on multi-line code statements embedded
in lambdas - but I feel like we did enough restructuring
for this fix now ;-)

 From  your other mail:
> Oh, yes. I missed Chris' comment. Perhaps just add a comment about that for posterity?

I added the following comment to stress out that
a reachability fence is not needed (at the two places
where it occurred):

  479         // add new Level.
  480         Level levelObject = new Level(name, x);
  481         // There's no need to use a reachability fence here because
  482         // KnownLevel keeps a strong reference on the level when
  483         // level.getClass() == Level.class.
  484         return KnownLevel.findByValue(x, KnownLevel::referent).get();

For the record here are the changes - including Mandy's suggestions.
If there's no objection I will push them after testing has passed,
since Mandy said she didn't require a new webrev.

http://cr.openjdk.java.net/~dfuchs/webrev_6543126/webrev.03

best regards,

-- daniel

On 16/08/16 14:54, Peter Levart wrote:
> Hi Daniel,
>
> Now that you have various findByXXX methods return Optional<Level>, you
> could make methods that use them more Optional-API-powered. For example,
> findLevel could be written as a single expression:
>
>     static Level findLevel(String name) {
>         return KnownLevel
>             .findByName(Objects.requireNonNull(name), KnownLevel::mirrored)
>             .or(() -> {
>                 // Now, check if the given name is an integer.  If so,
>                 // first look for a Level with the given value and then
>                 // if necessary create one.
>                 try {
>                     int x = Integer.parseInt(name);
>                     return KnownLevel
>                         .findByValue(x, KnownLevel::mirrored)
>                         .or(() -> {
>                             // add new Level
>                             new Level(name, x);
>                             return KnownLevel.findByValue(x,
> KnownLevel::mirrored);
>                         });
>                 } catch (NumberFormatException ex) {
>                     // Not an integer.
>                     // Drop through.
>                     return Optional.empty();
>                 }
>             })
>             .or(() -> KnownLevel.findByLocalizedLevelName(name,
> KnownLevel::mirrored))
>             .orElse(null);
>     }
>
>
> Same with parse:
>
>     public static synchronized Level parse(String name) throws
> IllegalArgumentException {
>         return KnownLevel
>             // Look for a known Level with the given non-localized name.
>             .findByName(Objects.requireNonNull(name), KnownLevel::referent)
>             .or(() -> {
>                 // Now, check if the given name is an integer.  If so,
>                 // first look for a Level with the given value and then
>                 // if necessary create one.
>                 try {
>                     int x = Integer.parseInt(name);
>                     return KnownLevel
>                         .findByValue(x, KnownLevel::referent)
>                         .or(() -> {
>                             // add new Level.
>                             new Level(name, x);
>                             return KnownLevel.findByValue(x,
> KnownLevel::referent);
>                         });
>                 } catch (NumberFormatException ex) {
>                     // Not an integer.
>                     // Drop through.
>                     return Optional.empty();
>                 }
>             })
>             // Finally, look for a known level with the given localized
> name,
>             // in the current default locale.
>             // This is relatively expensive, but not excessively so.
>             .or(() -> KnownLevel.findByLocalizedLevelName(name,
> KnownLevel::referent))
>             // OK, we've tried everything and failed
>             .orElseThrow(() -> new IllegalArgumentException("Bad level
> \"" + name + "\""));
>     }
>
>
> 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
>>>
>>
>