Re: <Swing Dev> [10] RFR JDK-8175968: The javax.swing.filechooser.FileSystemView constructor consumes memory by adding a PropertyChangeListener that is never removed.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] RFR JDK-8175968: The javax.swing.filechooser.FileSystemView constructor consumes memory by adding a PropertyChangeListener that is never removed.

Sergey Bylokhov
Hi,
I would like to propose a new review as a solution which I think should solve the problem:
Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
Webrev can be found at: http://cr.openjdk.java.net/~serb/8175968/webrev.00

----- [hidden email] wrote:

> Hi all,
>
>  > but imho it still beats adding an extra method to the API
>  > and shifting the burden to the API user.
>
> Just a question, would you be an "API user" of this method
> directly? It seems to me that dispose() could be called somewhere
> knowing more about the lifetime of this object (possibly from
> BasicFileChooserUI) so that it remains an implementation
> detail to most users of JFileChooser.
>
> The WeakReference trick would then just be a (poor) fallback
> mechanism.
>
> So this trick (introduced somewhere between Java 6 and 8)
> never worked. As mentioned, the key is to ensure the listeneris
> a static inner class. For example, this works:
>
>      private PropertyChangeListener pcl;
>
>      public FileSystemView() {
>          pcl = createPropertyChangeListener();
>      }
>
>      private static createPropertyChangeListener() {
>           // current content of constructor goes here
>      }
>
> The 'static' is all-important, so that the anonymous listener class
> becomes static also, and doesn't have a synthetic reference to
> the outer class.
>
>
>  > Thanks to the weak listener, the FileSystemView will get GC-ed as
>  > the listener no longer keeps a hard reference to it.
>
> I don't think a Weak Listener helps - the code you referenced earlier
> suffers the same limitation as the current code: The reference
> isn't freedunless a new property change event comes along. Property
> changeevents are very infrequent from UIManager - typically never.
>
>
> I've also been struggling to see a good alternative to dispose().
> Finalizers came to mind also but ... (UPDATE: deprecated now huh?
> OK forget it then.)
>
> An alternative could be a timer, but that's a lot of extra baggage.
>
> IF THIS WERE APPLICATION CODE a trick like this might work,
> removing the need for both the listener and dispose():
>
>      private Boolean useSystemExtensionHiding = null;
>
>      private boolean getUseSystemExtensionHiding() {
>
>          assert SwingUtilities.isEventDispatchThread();
>
>          if (useSystemExtensionHiding == null) {
>              useSystemExtensionHiding =
>                
> UIManager.getDefaults().getBoolean("FileChooser.useSystemExtensionHiding");
>
>              SwingUtilities.invokeLater(() -> useSystemExtensionHiding
> = null);
>          }
>
>          return useSystemExtensionHiding;
>      }
>
> After all, the caching is just an optimisation to avoid repeated
> map-lookups during painting (right?) The above reduces it to
> once per full component paint, which is a small cost.
>
> But being library code I'm not sure if it's safe to make such
> assumptions (that the EDT event loop is running, for example).
> (Expert opinion?)
>
>
> ACTUALLY if avoiding dispose() is preferred - then I wonder if the
> optimisation is "past its expiry date"? The "free lunch may be
> over" now, but it was still being delivered since Java 1.1 :-)
>
> What is the cost of one map lookup (per file) compared to the cost
> of painting an icon for each file, for example? I'd guess they're
> about on-par.
>
> Would it be so bad to just drop the caching of this boolean
> altogether?
>
> Kind regards,
> Luke
>
>
>
> On 07/07/2017 09:21, Robin Stevens wrote:
> > I still do not think you need a public dispose method.
> > If you use the weak listener (the correct implementation), the
> > FileSystemView instance can be GC-ed, even when the
> > PropertyChangeListener is still attached to the UIManager.
> >
> > What you can do is:
> > - Store a reference to the PropertyChangeListener in the
> > FileSystemView class as a field.
> > - Override the finalize method of the FileSystemView class to
> > de-register the PropertyChangeListener from the UIManager. Thanks to
>
> > the weak listener, the FileSystemView will get GC-ed as the listener
>
> > no longer keeps a hard reference to it.
> >
> > Not the nicest way to do such cleanup (which should probably happen
> on
> > the EDT) in a finalizer, but imho it still beats adding an extra
> > method to the API and shifting the burden to the API user.
> >
> > Robin
> >
> > On Thu, Jul 6, 2017 at 3:15 PM, Prasanta Sadhukhan
> > <[hidden email]
> > <mailto:[hidden email]>>wrote:
> >
> >     Hi Sergey,
> >
> >     I tried with the proposed WeakPropertyChangeListener but as far
> I
> >     understand & tested, it seems also to rely on propertyChange()
> API
> >     to be called, so the scenario mentioned in the bug will still
> fail.
> >     I cannot find any other way rather than calling dispose() for
> the
> >     scenario mentioned there.
> >
> >     Regards
> >     Prasanta
> >
> >     On 7/5/2017 4:35 AM, Sergey Bylokhov wrote:
> >
> >         Hi, Prasanta.
> >         Probably it is possible to implement it without users
> >         interaction and new api?
> >
> >         ----- [hidden email]
> >         <mailto:[hidden email]>wrote:
> >
> >             Hi All,
> >
> >             Please review a fix for a memory leak issue where
> >             PropertyChangeListener
> >             object added by FileSystemView constructor is never
> removed.
> >             Proposed fix is to add dispose() method to be called by
> >             app when they
> >
> >             want to remove this resource.
> >
> >             Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
> >             <https://bugs.openjdk.java.net/browse/JDK-8175968>
> >             webrev:
> >            
> http://cr.openjdk.java.net/~psadhukhan/8175968/webrev.00/
> >            
> <http://cr.openjdk.java.net/%7Epsadhukhan/8175968/webrev.00/>
> >
> >             Regards
> >             Prasanta
> >
> >
> >
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] RFR JDK-8175968: The javax.swing.filechooser.FileSystemView constructor consumes memory by adding a PropertyChangeListener that is never removed.

Sergey Bylokhov
Any volunteers to review?
Thank you =)

On 8/27/17 14:19, Sergey Bylokhov wrote:

> Hi,
> I would like to propose a new review as a solution which I think should solve the problem:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
> Webrev can be found at: http://cr.openjdk.java.net/~serb/8175968/webrev.00
>
> ----- [hidden email] wrote:
>
>> Hi all,
>>
>>   > but imho it still beats adding an extra method to the API
>>   > and shifting the burden to the API user.
>>
>> Just a question, would you be an "API user" of this method
>> directly? It seems to me that dispose() could be called somewhere
>> knowing more about the lifetime of this object (possibly from
>> BasicFileChooserUI) so that it remains an implementation
>> detail to most users of JFileChooser.
>>
>> The WeakReference trick would then just be a (poor) fallback
>> mechanism.
>>
>> So this trick (introduced somewhere between Java 6 and 8)
>> never worked. As mentioned, the key is to ensure the listeneris
>> a static inner class. For example, this works:
>>
>>       private PropertyChangeListener pcl;
>>
>>       public FileSystemView() {
>>           pcl = createPropertyChangeListener();
>>       }
>>
>>       private static createPropertyChangeListener() {
>>            // current content of constructor goes here
>>       }
>>
>> The 'static' is all-important, so that the anonymous listener class
>> becomes static also, and doesn't have a synthetic reference to
>> the outer class.
>>
>>
>>   > Thanks to the weak listener, the FileSystemView will get GC-ed as
>>   > the listener no longer keeps a hard reference to it.
>>
>> I don't think a Weak Listener helps - the code you referenced earlier
>> suffers the same limitation as the current code: The reference
>> isn't freedunless a new property change event comes along. Property
>> changeevents are very infrequent from UIManager - typically never.
>>
>>
>> I've also been struggling to see a good alternative to dispose().
>> Finalizers came to mind also but ... (UPDATE: deprecated now huh?
>> OK forget it then.)
>>
>> An alternative could be a timer, but that's a lot of extra baggage.
>>
>> IF THIS WERE APPLICATION CODE a trick like this might work,
>> removing the need for both the listener and dispose():
>>
>>       private Boolean useSystemExtensionHiding = null;
>>
>>       private boolean getUseSystemExtensionHiding() {
>>
>>           assert SwingUtilities.isEventDispatchThread();
>>
>>           if (useSystemExtensionHiding == null) {
>>               useSystemExtensionHiding =
>>                  
>> UIManager.getDefaults().getBoolean("FileChooser.useSystemExtensionHiding");
>>
>>               SwingUtilities.invokeLater(() -> useSystemExtensionHiding
>> = null);
>>           }
>>
>>           return useSystemExtensionHiding;
>>       }
>>
>> After all, the caching is just an optimisation to avoid repeated
>> map-lookups during painting (right?) The above reduces it to
>> once per full component paint, which is a small cost.
>>
>> But being library code I'm not sure if it's safe to make such
>> assumptions (that the EDT event loop is running, for example).
>> (Expert opinion?)
>>
>>
>> ACTUALLY if avoiding dispose() is preferred - then I wonder if the
>> optimisation is "past its expiry date"? The "free lunch may be
>> over" now, but it was still being delivered since Java 1.1 :-)
>>
>> What is the cost of one map lookup (per file) compared to the cost
>> of painting an icon for each file, for example? I'd guess they're
>> about on-par.
>>
>> Would it be so bad to just drop the caching of this boolean
>> altogether?
>>
>> Kind regards,
>> Luke
>>
>>
>>
>> On 07/07/2017 09:21, Robin Stevens wrote:
>>> I still do not think you need a public dispose method.
>>> If you use the weak listener (the correct implementation), the
>>> FileSystemView instance can be GC-ed, even when the
>>> PropertyChangeListener is still attached to the UIManager.
>>>
>>> What you can do is:
>>> - Store a reference to the PropertyChangeListener in the
>>> FileSystemView class as a field.
>>> - Override the finalize method of the FileSystemView class to
>>> de-register the PropertyChangeListener from the UIManager. Thanks to
>>
>>> the weak listener, the FileSystemView will get GC-ed as the listener
>>
>>> no longer keeps a hard reference to it.
>>>
>>> Not the nicest way to do such cleanup (which should probably happen
>> on
>>> the EDT) in a finalizer, but imho it still beats adding an extra
>>> method to the API and shifting the burden to the API user.
>>>
>>> Robin
>>>
>>> On Thu, Jul 6, 2017 at 3:15 PM, Prasanta Sadhukhan
>>> <[hidden email]
>>> <mailto:[hidden email]>>wrote:
>>>
>>>      Hi Sergey,
>>>
>>>      I tried with the proposed WeakPropertyChangeListener but as far
>> I
>>>      understand & tested, it seems also to rely on propertyChange()
>> API
>>>      to be called, so the scenario mentioned in the bug will still
>> fail.
>>>      I cannot find any other way rather than calling dispose() for
>> the
>>>      scenario mentioned there.
>>>
>>>      Regards
>>>      Prasanta
>>>
>>>      On 7/5/2017 4:35 AM, Sergey Bylokhov wrote:
>>>
>>>          Hi, Prasanta.
>>>          Probably it is possible to implement it without users
>>>          interaction and new api?
>>>
>>>          ----- [hidden email]
>>>          <mailto:[hidden email]>wrote:
>>>
>>>              Hi All,
>>>
>>>              Please review a fix for a memory leak issue where
>>>              PropertyChangeListener
>>>              object added by FileSystemView constructor is never
>> removed.
>>>              Proposed fix is to add dispose() method to be called by
>>>              app when they
>>>
>>>              want to remove this resource.
>>>
>>>              Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
>>>              <https://bugs.openjdk.java.net/browse/JDK-8175968>
>>>              webrev:
>>>            
>> http://cr.openjdk.java.net/~psadhukhan/8175968/webrev.00/
>>>            
>> <http://cr.openjdk.java.net/%7Epsadhukhan/8175968/webrev.00/>
>>>
>>>              Regards
>>>              Prasanta
>>>
>>>
>>>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <Swing Dev> [10] RFR JDK-8175968: The javax.swing.filechooser.FileSystemView constructor consumes memory by adding a PropertyChangeListener that is never removed.

Alexander Zvegintsev
Looks fine.

Thanks,
Alexander.

On 03/09/2017 07:01, Sergey Bylokhov wrote:

> Any volunteers to review?
> Thank you =)
>
> On 8/27/17 14:19, Sergey Bylokhov wrote:
>> Hi,
>> I would like to propose a new review as a solution which I think
>> should solve the problem:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
>> Webrev can be found at:
>> http://cr.openjdk.java.net/~serb/8175968/webrev.00
>>
>> ----- [hidden email] wrote:
>>
>>> Hi all,
>>>
>>>   > but imho it still beats adding an extra method to the API
>>>   > and shifting the burden to the API user.
>>>
>>> Just a question, would you be an "API user" of this method
>>> directly? It seems to me that dispose() could be called somewhere
>>> knowing more about the lifetime of this object (possibly from
>>> BasicFileChooserUI) so that it remains an implementation
>>> detail to most users of JFileChooser.
>>>
>>> The WeakReference trick would then just be a (poor) fallback
>>> mechanism.
>>>
>>> So this trick (introduced somewhere between Java 6 and 8)
>>> never worked. As mentioned, the key is to ensure the listeneris
>>> a static inner class. For example, this works:
>>>
>>>       private PropertyChangeListener pcl;
>>>
>>>       public FileSystemView() {
>>>           pcl = createPropertyChangeListener();
>>>       }
>>>
>>>       private static createPropertyChangeListener() {
>>>            // current content of constructor goes here
>>>       }
>>>
>>> The 'static' is all-important, so that the anonymous listener class
>>> becomes static also, and doesn't have a synthetic reference to
>>> the outer class.
>>>
>>>
>>>   > Thanks to the weak listener, the FileSystemView will get GC-ed as
>>>   > the listener no longer keeps a hard reference to it.
>>>
>>> I don't think a Weak Listener helps - the code you referenced earlier
>>> suffers the same limitation as the current code: The reference
>>> isn't freedunless a new property change event comes along. Property
>>> changeevents are very infrequent from UIManager - typically never.
>>>
>>>
>>> I've also been struggling to see a good alternative to dispose().
>>> Finalizers came to mind also but ... (UPDATE: deprecated now huh?
>>> OK forget it then.)
>>>
>>> An alternative could be a timer, but that's a lot of extra baggage.
>>>
>>> IF THIS WERE APPLICATION CODE a trick like this might work,
>>> removing the need for both the listener and dispose():
>>>
>>>       private Boolean useSystemExtensionHiding = null;
>>>
>>>       private boolean getUseSystemExtensionHiding() {
>>>
>>>           assert SwingUtilities.isEventDispatchThread();
>>>
>>>           if (useSystemExtensionHiding == null) {
>>>               useSystemExtensionHiding =
>>> UIManager.getDefaults().getBoolean("FileChooser.useSystemExtensionHiding");
>>>
>>>               SwingUtilities.invokeLater(() -> useSystemExtensionHiding
>>> = null);
>>>           }
>>>
>>>           return useSystemExtensionHiding;
>>>       }
>>>
>>> After all, the caching is just an optimisation to avoid repeated
>>> map-lookups during painting (right?) The above reduces it to
>>> once per full component paint, which is a small cost.
>>>
>>> But being library code I'm not sure if it's safe to make such
>>> assumptions (that the EDT event loop is running, for example).
>>> (Expert opinion?)
>>>
>>>
>>> ACTUALLY if avoiding dispose() is preferred - then I wonder if the
>>> optimisation is "past its expiry date"? The "free lunch may be
>>> over" now, but it was still being delivered since Java 1.1 :-)
>>>
>>> What is the cost of one map lookup (per file) compared to the cost
>>> of painting an icon for each file, for example? I'd guess they're
>>> about on-par.
>>>
>>> Would it be so bad to just drop the caching of this boolean
>>> altogether?
>>>
>>> Kind regards,
>>> Luke
>>>
>>>
>>>
>>> On 07/07/2017 09:21, Robin Stevens wrote:
>>>> I still do not think you need a public dispose method.
>>>> If you use the weak listener (the correct implementation), the
>>>> FileSystemView instance can be GC-ed, even when the
>>>> PropertyChangeListener is still attached to the UIManager.
>>>>
>>>> What you can do is:
>>>> - Store a reference to the PropertyChangeListener in the
>>>> FileSystemView class as a field.
>>>> - Override the finalize method of the FileSystemView class to
>>>> de-register the PropertyChangeListener from the UIManager. Thanks to
>>>
>>>> the weak listener, the FileSystemView will get GC-ed as the listener
>>>
>>>> no longer keeps a hard reference to it.
>>>>
>>>> Not the nicest way to do such cleanup (which should probably happen
>>> on
>>>> the EDT) in a finalizer, but imho it still beats adding an extra
>>>> method to the API and shifting the burden to the API user.
>>>>
>>>> Robin
>>>>
>>>> On Thu, Jul 6, 2017 at 3:15 PM, Prasanta Sadhukhan
>>>> <[hidden email]
>>>> <mailto:[hidden email]>>wrote:
>>>>
>>>>      Hi Sergey,
>>>>
>>>>      I tried with the proposed WeakPropertyChangeListener but as far
>>> I
>>>>      understand & tested, it seems also to rely on propertyChange()
>>> API
>>>>      to be called, so the scenario mentioned in the bug will still
>>> fail.
>>>>      I cannot find any other way rather than calling dispose() for
>>> the
>>>>      scenario mentioned there.
>>>>
>>>>      Regards
>>>>      Prasanta
>>>>
>>>>      On 7/5/2017 4:35 AM, Sergey Bylokhov wrote:
>>>>
>>>>          Hi, Prasanta.
>>>>          Probably it is possible to implement it without users
>>>>          interaction and new api?
>>>>
>>>>          ----- [hidden email]
>>>>          <mailto:[hidden email]>wrote:
>>>>
>>>>              Hi All,
>>>>
>>>>              Please review a fix for a memory leak issue where
>>>>              PropertyChangeListener
>>>>              object added by FileSystemView constructor is never
>>> removed.
>>>>              Proposed fix is to add dispose() method to be called by
>>>>              app when they
>>>>
>>>>              want to remove this resource.
>>>>
>>>>              Bug: https://bugs.openjdk.java.net/browse/JDK-8175968
>>>> <https://bugs.openjdk.java.net/browse/JDK-8175968>
>>>>              webrev:
>>> http://cr.openjdk.java.net/~psadhukhan/8175968/webrev.00/
>>> <http://cr.openjdk.java.net/%7Epsadhukhan/8175968/webrev.00/>
>>>>
>>>>              Regards
>>>>              Prasanta
>>>>
>>>>
>>>>
>
>