RFR: 6543126: Level.known can leak memory
Daniel Fuchs
daniel.fuchs at oracle.com
Wed Aug 17 12:53:56 UTC 2016
Hi Peter,
On 17/08/16 12:20, Peter Levart wrote:
> Hi Daniel,
>
> Thinking of this patch from the compatibility standpoint, aren't you
> afraid that someone might be using the following idiom:
>
> public class MyLevel extends java.util.logging.Level {
> static {
> new MyLevel("LEVEL1", 1);
> new MyLevel("LEVEL2", 2);
> ...
> }
>
> public static void init() {}
>
> private MyLevel(String name, int value) {
> super(name, value);
> }
> ...
> }
>
> ...and then use those levels in code like:
>
> MyLevel.init();
> ...
> Level level = Level.parse("LEVEL1") ...
That's a possibility, and maybe that would deserve to be noted as
a side effect of the fix in the release notes.
I do not believe this is something we would want to actively support
though: the usual idiom for custom level would rather be to define
them as public final static constant, and use a direct reference
in the code.
Something like:
public class MyLevel extends Level {
public static final MyLevel LEVEL1 = new MyLevel("LEVEL1", 1);
...
}
Level level = MyLevel.LEVEL1;
or even more simpler:
public static class Levels {
public static Level LEVEL1 = new Level("LEVEL1", 1) {};
...
}
So maybe this is a risk we should take. Mandy, what do you think?
The other alternative is not to fix the bug as it's been here
for several releases ;-)
-- daniel
>
>
> With the proposed patch, this will not work any more as custom Level
> subclass instances will possibly be GCed before being "parsed" and
> retained.
>
> To prevent that from happening and still allow custom Level subclasses
> and their class loaders to be garbage-collectable, I recommend that you
> "pin" each constructed subclass instance to its ClassLoader with a
> strong reference. You could use a ClassLoaderValue for this purpose,
> like in the following addition to your patch (see KnownLevel.add):
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Level.known/webrev.01/
>
> 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
>>>
>>
>
More information about the core-libs-dev
mailing list