Modify

Opened 7 weeks ago

Closed 3 weeks ago

#17040 closed defect (fixed)

Memory leaks

Reported by: GerdP Owned by: team
Priority: normal Milestone: 18.12
Component: Core Version:
Keywords: template_report memory Cc: skyper

Description

What steps will reproduce the problem?

  1. Download OSM data
  2. Press Ctrl+H to see history of a selected object
  3. Close all data layers
  4. Use VisualVM to create a heapdump after a GC

What is the expected result?

No instances of VersionInfoPanel

What happens instead?

Some instances of this class, also various other instances of josm.gui.history.*

Please provide any additional information below. Attach a screenshot if possible.

Build-Date:2018-11-26 18:47:05
Revision:14455
Is-Local-Build:true

Identification: JOSM/1.5 (14455 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 4871 MB / 5461 MB (4694 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080

Plugins:
+ FastDraw (34510)
+ apache-commons (34506)
+ buildings_tools (34724)
+ download_along (34503)
+ editgpx (34737)
+ ejml (34389)
+ geotools (34513)
+ gpxfilter (34506)
+ jaxb (34506)
+ jts (34524)
+ measurement (34529)
+ o5m (34405)
+ opendata (34698)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ utilsplugin2 (34506)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

Attachments (21)

17040_1.png (24.1 KB) - added by Don-vip 7 weeks ago.
17040_2.png (20.7 KB) - added by Don-vip 7 weeks ago.
17040-work.patch (623 bytes) - added by GerdP 7 weeks ago.
17040-work2.patch (2.5 KB) - added by GerdP 7 weeks ago.
17040-work3.patch (3.7 KB) - added by GerdP 7 weeks ago.
17040-work4.patch (5.4 KB) - added by GerdP 7 weeks ago.
17040-work5.patch (7.6 KB) - added by GerdP 7 weeks ago.
now also clear lists containing TestError Instances, they prevent GC of class Dataset
17040-work6.patch (10.9 KB) - added by GerdP 7 weeks ago.
fix more leaks reg. Dataset
17040-dataset.patch (511 bytes) - added by GerdP 7 weeks ago.
@Don-vip: Why isn't Dataset.clear() called as in this patch?
copy+paste.png (13.7 KB) - added by GerdP 7 weeks ago.
DrawAction.PNG (23.4 KB) - added by GerdP 7 weeks ago.
I wonder why DrawAction.finishDrawing() is called so rarely. It is not even called when the edit layer is closed. Where would be the best place to change that?
17040-DrawAction.patch (825 bytes) - added by GerdP 7 weeks ago.
Please review
17040-properties.patch (1.3 KB) - added by GerdP 7 weeks ago.
Please review
17040-properties-v2.patch (1.6 KB) - added by GerdP 7 weeks ago.
Fixed NPE (sel was reset too early)
kitfox.patch (725 bytes) - added by GerdP 7 weeks ago.
Optimization: Reduce memory footprint and number of String instances für svg graphics
ChangesetDetail.patch (2.5 KB) - added by GerdP 6 weeks ago.
Please review, I am always unsure whether to use destroy() or dispose(). Removes two listeners added for each ChangesetManager (Ctrl+Alt+C)
ChangesetDetail-v2.patch (4.0 KB) - added by GerdP 6 weeks ago.
17040-toggle-dlg.patch (3.6 KB) - added by GerdP 4 weeks ago.
17040-reltoolbox.patch (506 bytes) - added by GerdP 4 weeks ago.
17040-pref-dlg.patch (1.7 KB) - added by GerdP 4 weeks ago.
17040-pref-dlg-v2.patch (5.1 KB) - added by GerdP 3 weeks ago.
OT: simplify PreferenceDialog

Download all attachments as: .zip

Change History (119)

comment:1 Changed 7 weeks ago by Don-vip

Milestone: 18.12

comment:2 Changed 7 weeks ago by GerdP

Owner: changed from GerdP to team
Status: assignednew

I tried to find out what's wrong but my understanding of dialog components is too small. GC doesn't remove the HistoryBrowserDialog instance once the dialog is closed.

Changed 7 weeks ago by Don-vip

Attachment: 17040_1.png added

Changed 7 weeks ago by Don-vip

Attachment: 17040_2.png added

comment:3 Changed 7 weeks ago by Don-vip

To understand memory leaks you need to use the "GC root" feature of VisualVM. For a given class instance it will show you the root objects that keep a chain of references to this object, preventing it to be garbage collected.

In this case we have 8 instances of VersionInfoPanel, with two different chains:



In both cases TaginfoAction is involved. JosmActions are natively Destroyable and will release all resources (including listeners) when their destroy method is called. In this case we need to make sure that HistoryBrowserDialog.dispose() effectively calls the destroy method of all inner actions.

Last edited 7 weeks ago by Don-vip (previous) (diff)

comment:4 Changed 7 weeks ago by Don-vip

Owner: changed from team to Don-vip
Status: newassigned

comment:5 Changed 7 weeks ago by GerdP

OK, thanks for the hint. I think there is also a problem with the Dataset class. My understanding is that JOSM should keep only one instance when no layer is displayed. Sometimes I see a lot more and
ds in org.openstreetmap.josm.gui.tagging.ac.AutoCompletionManager seems to be involved.

comment:6 Changed 7 weeks ago by Don-vip

Yes it's easier to add memory leaks than remove them, I'm sure we still have plenty of them. You can investigate the Dataset/AutoCompletionManager case, I am only working on the history stuff right now.

comment:7 Changed 7 weeks ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 14463/josm:

fix #17040 - fix memory leaks when calling history dialog

comment:8 Changed 7 weeks ago by Don-vip

In 14465/josm:

see #17040 - fix PMD warning

comment:9 Changed 7 weeks ago by GerdP

Wow, quite a big change. No wonder that I wasn't able to fix that ;)
The good thing is that I cannot reproduce the Dataset/AutoCompletionManager leak with r14465 :)

comment:10 in reply to:  9 Changed 7 weeks ago by stoecker

Replying to GerdP:

Wow, quite a big change. No wonder that I wasn't able to fix that ;)

Also a nice one. I like it when improving the code is done by removing code without loosing functionality. The old code thus had unneeded complexity.

comment:11 Changed 7 weeks ago by GerdP

I still see a leak where SideButton and NotesDialog are involved. Should I open a new ticket for that?

comment:12 in reply to:  11 Changed 7 weeks ago by stoecker

Replying to GerdP:

I still see a leak where SideButton and NotesDialog are involved. Should I open a new ticket for that?

Yes. For each. Or simply fix them without ticket :-)

comment:13 Changed 7 weeks ago by GerdP

I'll give it another try later. It seems that the ToggleDialogs are never destroyed. I tried to change that and got problems with the LayerManager.
I wonder if we have some experimental code active. I see that MainPanel.reAddListerners() is called although there is this comment

   /**
     * Re-adds the layer listeners. Never call this in production, only needed for testing.
     */
Last edited 7 weeks ago by Don-vip (previous) (diff)

comment:14 Changed 7 weeks ago by stoecker

In case you don't know: The wiki has an Annotation feature:

browser/josm/trunk/src/org/openstreetmap/josm/gui/MainPanel.java

That's extremely helpful to find when (and why) certain lines have been added or modified.

comment:15 Changed 7 weeks ago by GerdP

Yes, blame is very useful. I use TortoiseSVN for that. Anyhow, I still have no idea what's wrong :(
I simply have no idea how it should look when everything is correct. Maybe you can try to fix the problem with the ToggleDialogs as well and I'll try to learn from your changes?

Changed 7 weeks ago by GerdP

Attachment: 17040-work.patch added

comment:16 Changed 7 weeks ago by GerdP

I've added a small patch that seems to fix one leak.

comment:17 Changed 7 weeks ago by Don-vip

Good catch, I didn't check for other TaginfoAction outside of history browser.

comment:18 Changed 7 weeks ago by Don-vip

Resolution: fixed
Status: closedreopened
Summary: Memory leak VersionInfoPanelMemory leaks

We can keep this ticket open until we release 18.12 or we manage to fix all memory leaks :D

Changed 7 weeks ago by GerdP

Attachment: 17040-work2.patch added

comment:19 Changed 7 weeks ago by GerdP

17040-work2.patch fixes more problems, esp. mutliple MapView instances which kept refs to all the ToggleDialogs.
I guess this can be done better, the patch just points out some problems.

Last edited 7 weeks ago by GerdP (previous) (diff)

Changed 7 weeks ago by GerdP

Attachment: 17040-work3.patch added

comment:20 Changed 7 weeks ago by GerdP

17040-work3.patch also fixes a leak in NotesDialog. I think I am starting to undertstand how to use the results in VisualVM to find out where the problems are. I just don't know for sure where to fix them. I'll continue searching tomorrow.

Changed 7 weeks ago by GerdP

Attachment: 17040-work4.patch added

comment:21 Changed 7 weeks ago by GerdP

With 17040-work4.patch two more leaks reg. RelationListEditor and SaveLayersDialog are fixed.

Changed 7 weeks ago by GerdP

Attachment: 17040-work5.patch added

now also clear lists containing TestError Instances, they prevent GC of class Dataset

Changed 7 weeks ago by GerdP

Attachment: 17040-work6.patch added

fix more leaks reg. Dataset

comment:22 Changed 7 weeks ago by stoecker

A small note. The fact that you have commit permissions means that we trust you. No need to ask for review in fixing bugs. Only necessary if you don't trust your own ideas.

If you make an error we can always blame you later :-)

No really, we don't want broken nightly releases, but there is no reason to fear it in case you are not 100% sure...

comment:23 Changed 7 weeks ago by GerdP

Well, I don't trust some of my solutions. Esp. the one reg. NavigatableComponent.
I did not yet try to run the unit tests. I am creating the patches on my couch using a rather slow Laptop ;-)

comment:24 Changed 7 weeks ago by GerdP

In 14470/josm:

see #17040 Fix various memory leaks
Not sure if this will break Unit tests. Many of them don't work on my PC with a clean copy.

comment:25 Changed 7 weeks ago by GerdP

Need help: Some tests fail but I have no idea why:
https://josm.openstreetmap.de/jenkins/job/JOSM/lastCompletedBuild/testReport/

comment:26 Changed 7 weeks ago by GerdP

In 14471/josm:

see #17040 Move call of UndoRedoHandler.removeCommandQueueListener() to the action that adds itself

comment:27 in reply to:  25 ; Changed 7 weeks ago by Don-vip

Replying to GerdP:

Some tests fail but I have no idea why:

See #16973. This is unrelated to your changes but a side effect of the server migration.

comment:28 Changed 7 weeks ago by Don-vip

Owner: changed from Don-vip to team
Status: reopenednew

comment:29 in reply to:  27 Changed 7 weeks ago by GerdP

Replying to Don-vip:

Replying to GerdP:

Some tests fail but I have no idea why:

See #16973. This is unrelated to your changes but a side effect of the server migration.

You are right, but I was aware of #16973. These were other erros, but but they are gone again with r14471.

comment:30 Changed 7 weeks ago by GerdP

In 14472/josm:

see #17040 call removeZoomChangeListener in destroy()

comment:31 Changed 7 weeks ago by GerdP

In 14473/josm:

see #17040 don't keep refs to OSM primitives in validation tests because this can prevent GC of Dataset instance

comment:32 Changed 7 weeks ago by GerdP

In 14475/josm:

see #17040 Use PopupMenuHandler to reset all primitiveActions

comment:33 Changed 7 weeks ago by GerdP

In 14477/josm:

see #17040 fix checkstyle issues

comment:34 Changed 7 weeks ago by GerdP

In 14478/josm:

see #17040 make MapSlider Destroyable

Changed 7 weeks ago by GerdP

Attachment: 17040-dataset.patch added

@Don-vip: Why isn't Dataset.clear() called as in this patch?

comment:35 Changed 7 weeks ago by GerdP

Sorry, I've asked that before and now found #13394. I think there is no need to keep a big memory leak when VisualVM also shows a small one. Without the Dataset.clear() call I sometimes find Dataset instances without any GC root. Maybe GC fails because each primitve has a ref back to Dataset?

comment:36 Changed 7 weeks ago by GerdP

In 14479/josm:

see #17040 Clean refs to Dataset in AutoCompletionManager when OsmDataLayer is removed

Changed 7 weeks ago by GerdP

Attachment: copy+paste.png added

comment:37 Changed 7 weeks ago by GerdP

I found one more leak but I have no idea how to fix that. I used copy+paste to create a copy of a leisure=pitch way, later I used upload and closed the data layer. A heapdump shows as GC root for the Dataset instance.

comment:38 Changed 7 weeks ago by Don-vip

In 14483/josm:

see #17040 - fix SonarQube issues

comment:39 Changed 7 weeks ago by Don-vip

In 14500/josm:

see #17040 - fix memory leak after primitives are copied to clipboard

comment:40 Changed 7 weeks ago by stoecker

I'd say #17064 is a regression due to this ticket.

comment:41 Changed 7 weeks ago by GerdP

Yes, working on it.

Changed 7 weeks ago by GerdP

Attachment: DrawAction.PNG added

I wonder why DrawAction.finishDrawing() is called so rarely. It is not even called when the edit layer is closed. Where would be the best place to change that?

Changed 7 weeks ago by GerdP

Attachment: 17040-DrawAction.patch added

Please review

Changed 7 weeks ago by GerdP

Attachment: 17040-properties.patch added

Please review

comment:42 Changed 7 weeks ago by GerdP

Please ignore 17040-properties.patch. Causes NPE.

Changed 7 weeks ago by GerdP

Attachment: 17040-properties-v2.patch added

Fixed NPE (sel was reset too early)

Changed 7 weeks ago by GerdP

Attachment: kitfox.patch added

Optimization: Reduce memory footprint and number of String instances für svg graphics

comment:43 Changed 6 weeks ago by Don-vip

resetSelection would be a better name. For the svgSalamander patch, can you please create a pull request as well to https://github.com/blackears/svgSalamander ?

comment:44 Changed 6 weeks ago by GerdP

OK, thanks for review. I've never created a pull request. What do I have to do?

comment:45 Changed 6 weeks ago by Don-vip

For simpler ones concerning only one file you can just use the edit button of Github and follow the wizards.

comment:46 Changed 6 weeks ago by GerdP

OK, I'll try. Besides that I wonder if we can forget the svg xml data once the image was rendered. Would that be possible?
Or do we need it again for other resolutions?

comment:47 Changed 6 weeks ago by GerdP

In 14509/josm:

see #17040 implement resetSelection() to forget recently selected primitives

comment:48 Changed 6 weeks ago by Don-vip

The XML is kept in memory?! Where?

comment:49 Changed 6 weeks ago by GerdP

Well, not the xml, but class StyleAttribute stores all kinds of properties as strings. That means we have thousands of String instances with value "0" or "1". My small patch catches many but not all of them.

comment:50 in reply to:  47 ; Changed 6 weeks ago by Klumbumbus

Replying to GerdP:

forget recently selected primitives

I don't undertstand the code, but the commit message sounds a bit as it might break wiki:/Help/Action/UndoSelection

comment:51 Changed 6 weeks ago by GerdP

In 14510/josm:

see #17040 call DrawAction.finishDrawing() when selection is empty or destroy() is called.

comment:52 Changed 6 weeks ago by GerdP

It resets a list in the TagEditor.

comment:53 in reply to:  50 ; Changed 6 weeks ago by GerdP

Replying to Klumbumbus:

Replying to GerdP:

forget recently selected primitives

I don't undertstand the code, but the commit message sounds a bit as it might break wiki:/Help/Action/UndoSelection

I just tried with r14460. It doesn't seem to work, or I don't understand what it should do.

comment:54 in reply to:  53 ; Changed 6 weeks ago by Klumbumbus

Cc: skyper added

Replying to GerdP:

It doesn't seem to work

When you select one or more object(s) and unselect them then use UndoSelection, you get back the previous selection. However for me this works only once and not consecutively like written on the wiki page. Maybe skyper remembers if this feature used to behave differently in the past (as he added the wiki text 8 years ago).

comment:55 in reply to:  49 Changed 6 weeks ago by Don-vip

Replying to GerdP:

Well, not the xml, but class StyleAttribute stores all kinds of properties as strings. That means we have thousands of String instances with value "0" or "1". My small patch catches many but not all of them.

These strings are probably needed, I don't know the svgSalamander code by heart and it's not really state of the art/easy to understand... We keep the SVGDiagram in ImageResource so that we can request images of different sizes after having loaded the SVG.

ImageResource are optionnally kept in actions:

    /**
     * Set both icons of an Action
     * @param a The action for the icons
     * @param addresource Adds an resource named "ImageResource" if <code>true</code>
     * @since 10369
     */
    public void attachImageIcon(AbstractAction a, boolean addresource) {
        attachImageIcon(a);
        if (addresource) {
            a.putValue("ImageResource", this);
        }
    }

It seems this feature is only used in SideButton constructor:

    /**
     * Constructs a new {@code SideButton}.
     * @param action action used to specify the new button
     * an icon must be provided with {@link ImageResource#attachImageIcon(AbstractAction, boolean)}
     * @throws IllegalArgumentException if no icon provided
     * @since 744
     */
    public SideButton(Action action) {
        super(action);
        ImageResource icon = (ImageResource) action.getValue("ImageResource");
        if (icon != null) {
            setIcon(icon.getImageIconBounded(
                ImageProvider.ImageSizes.SIDEBUTTON.getImageDimension()));
        } else {
            throw new IllegalArgumentException("No icon provided");
        }

Maybe we actually store some ImageResource instances for nothing by calling too often attachImageIcon(this, true) for places where we could maybe use simply attachImageIcon(this). Hard to tell, it needs to be checked case by case...

comment:56 in reply to:  54 Changed 6 weeks ago by GerdP

Replying to Klumbumbus:

Replying to GerdP:

It doesn't seem to work

When you select one or more object(s) and unselect them then use UndoSelection, you get back the previous selection. However for me this works only once and not consecutively like written on the wiki page. Maybe skyper remembers if this feature used to behave differently in the past (as he added the wiki text 8 years ago).

I think the problem is quite old. If I got that right we have to many selectionChanged events for one user action.
This also breaks the Shift+R (#14862).
Example: When you select an existing object this causes one SelectionChangeEvent for MousePressed and another for MouseReleased.

Last edited 6 weeks ago by GerdP (previous) (diff)

comment:57 Changed 6 weeks ago by GerdP

Forget comment:56, I was wrong. I've attached an incomplete patch to #14862.

comment:58 Changed 6 weeks ago by GerdP

In 14537/josm:

see #17040 call Dataset.clear() in OsmDataLayer.destroy()

Without this call sometimes the complete Dataset instance is not GCed, esp. when dDataset contains a large number of objects. The call to clear() eases work of GC.

comment:59 Changed 6 weeks ago by Don-vip

In 14542/josm:

see #17040 - fix unit tests

comment:60 Changed 6 weeks ago by GerdP

In 14545/josm:

see #17040 - destroy ChangesetInfoAction and UserInfoAction

Changed 6 weeks ago by GerdP

Attachment: ChangesetDetail.patch added

Please review, I am always unsure whether to use destroy() or dispose(). Removes two listeners added for each ChangesetManager (Ctrl+Alt+C)

comment:61 Changed 6 weeks ago by Don-vip

When dispose() is available (in Window subclasses) it should be used. When it is not, you can implement Destroyable.

comment:62 Changed 6 weeks ago by GerdP

My rule is this:

  • if I can override dispose() I should do that, next
  • if I can override destroy() I thould do that, next
  • implement Destroyable

Do you agree?

comment:63 Changed 6 weeks ago by Don-vip

Yes :)

comment:64 Changed 6 weeks ago by GerdP

In 14546/josm:

see #17040 implement Destroyable in ChangesetDetailPanel and use it

Removes listeners added with each use of ChangesetManager (Ctrl+Alt+C)

comment:65 Changed 6 weeks ago by GerdP

In 14547/josm:

see #17040 revert r14546 Sometimes tries to remove listener twice

comment:66 Changed 6 weeks ago by GerdP

In 14548/josm:

see #17040 - destroy AddImageryLayerAction

Up to now they were never destroyed, and one is created for each available entry in ImageryMenu whenever you open the menu.

Changed 6 weeks ago by GerdP

Attachment: ChangesetDetail-v2.patch added

comment:67 Changed 6 weeks ago by GerdP

Please review ChangesetDetail-v2.patch. I found no reason for the addComponentListener and it caused trouble with the previous patch.
Maybe it is needed for something special that I never use?

comment:68 Changed 6 weeks ago by Don-vip

The changeset window is non modal so it seems the code ensures the buttons are enabled only when the panel is displayed and there's a layer.

I don't think many people use the changeset manager, so it's probably fine to remove the listeners only on destroy (i.e when the window is closed) instead of hide/show (i.e when a tab is changed).

comment:69 Changed 6 weeks ago by GerdP

OK, I'll work with this patch for a while. With this last patch it looks quite good now, Dataset is no longer kept after "normal" edit sessions and listeners are removed. I'll play now with more complex cases like conflict dialog and undo/redo actions. I guess that some plugins also need a closer look.

comment:70 Changed 5 weeks ago by GerdP

In 14555/josm:

see #17040 commit ChangesetDetail-v2.patch

comment:71 Changed 5 weeks ago by GerdP

In 14556/josm:

see #17040 more cleanup in OsmDataLayer.destroy()

  • remove DatasetListeners previously added
  • clear recentRelations
  • don't call Dataset.clear() here because there may still be active listeners
  • call DataSet.clear in LayerManager after all listeners are removed

Hope this doesn't break unit tests, can't test on my machine.

comment:72 Changed 5 weeks ago by GerdP

Can anybody explain why it is not okay to use Dataset.clear() for a locked Dataset, please? I see no good reason for that.

Changed 4 weeks ago by GerdP

Attachment: 17040-toggle-dlg.patch added

comment:73 Changed 4 weeks ago by GerdP

Please review, I've removed a SwingUtilities.invokeLater() here to fix a leak. It was introduced with r3346 but for me it seems to work also without.

comment:74 Changed 4 weeks ago by GerdP

In 14589/josm:

#see #17040 implement Destroyable in ToggleDialog.TitleBar and use it in ToggleDialog.destroy()

Changed 4 weeks ago by GerdP

Attachment: 17040-reltoolbox.patch added

comment:75 Changed 4 weeks ago by GerdP

I tried to commit 17040-reltoolbox.patch but it seems I have no right to do that. Message would be

see #17040 call removeActiveLayerChangeListener() in destroy()


comment:76 Changed 4 weeks ago by Don-vip

The folder is named reltoolbox, no?

comment:77 Changed 4 weeks ago by GerdP

yes, why?
I see, the patch doesn't contain the plugin directory.

Last edited 4 weeks ago by GerdP (previous) (diff)

comment:78 Changed 4 weeks ago by GerdP

Seems the commit problem reg. 17040-reltoolbox.patch is on my side. I've now committed the patch on my PC, yesterday I tried with the laptop.
One question: I've changed ToggleDialog during the last days and this is a super class in some plugins. Should I compile those and commit only the josm/dist changes each time when I change ToggleDialog? My changes might have broken something in those plugins.

comment:79 Changed 4 weeks ago by GerdP

In 14598/josm:

see #17040 kitofx.patch: use String.intern() for deduplication of Strings that appear in svg graphics

comment:80 in reply to:  78 Changed 4 weeks ago by Don-vip

Replying to GerdP:

My changes might have broken something in those plugins.

It seems fine. The plugin integration test is OK and we have no bug reports.

comment:81 Changed 4 weeks ago by GerdP

Would the integration tests recognize a pointer which is set to null in the destroy() methods?
Or do they only check if plugins can be compiled?

comment:82 Changed 4 weeks ago by Don-vip

No, it checks binary compatibility only. To detect NPE you can take a look to sonar.

comment:83 Changed 4 weeks ago by GerdP

sonar shows a lot of interesting information but I found no hint that would help me in my case. I've now used some simple searches to find out if any of the plugins uses any of the fields that are set to null and found none, so I'm crossing fingers ;-)

comment:84 Changed 4 weeks ago by GerdP

There is one more leak involiving josm.data.preferences.ColorInfo. Each time when I press F12 I see 373 new instances. I did not yet find out where to fix that.

Changed 4 weeks ago by GerdP

Attachment: 17040-pref-dlg.patch added

comment:85 Changed 4 weeks ago by GerdP

Merry Christmas!
Please review 17040-pref-dlg.patch. Is it OK to call dispose() from the OK and Cancel action?

comment:86 Changed 4 weeks ago by Don-vip

We usually don't call dispose directly but the javadoc doesn't forbid it. Did you measure the time it takes to show again the preferences dialog after that? Some parts are slow to initialize and the performance penalty could maybe be seen as a regression for end users.

comment:87 Changed 4 weeks ago by GerdP

The dialog is created each time when you press F12. The change only removes the memory leak.

comment:88 Changed 4 weeks ago by GerdP

We have DownloadDialog that implements a singleton but PreferenceDialog doesn't. I don't see a performance problem but maybe plugins add something that increases complexity so that the delay is notable?

comment:89 Changed 3 weeks ago by Don-vip

DownloadDialog is a lot faster to load. Preferences dialog loadup time increases with the number of plugin installed, and also it can take time to switch between tabs (as tab initialization is delayed to make the dialog load faster). See #7386 for details.

comment:90 in reply to:  87 Changed 3 weeks ago by Don-vip

Replying to GerdP:

The dialog is created each time when you press F12. The change only removes the memory leak.

OK sorry I misunderstood the patch.

comment:91 Changed 3 weeks ago by GerdP

In 14601/josm:

see #17040 dispose PreferenceDialog and call removeWindowListener()

comment:92 Changed 3 weeks ago by GerdP

I'd prefer to use setDefaultCloseOperation(JDialog.DISPOSE_ON_CLOSE); but that would not allow to set the field canceled.
No idea why there is a getter for this field, it seems to be unused. Maybe legacy code?

comment:93 Changed 3 weeks ago by Don-vip

You can use the blame feature to know that.

Changed 3 weeks ago by GerdP

Attachment: 17040-pref-dlg-v2.patch added

OT: simplify PreferenceDialog

comment:94 in reply to:  93 Changed 3 weeks ago by GerdP

Replying to Don-vip:

You can use the blame feature to know that.

I used that before I wrote my comment;) With r2745 .. r2748 Gubaer implemented OAuth and in those changes the getter was introduced. I found no reason for that in those old sources nor in the current, but it's hard to tell because there may be plugins using it.
17040-pref-dlg-v2.patch shows a possible alternative. Please ignore the changes in GettingStarted, I disabled the Christmas code because it obstructed my measurements and forgot to revert.

comment:95 Changed 3 weeks ago by GerdP

In 14602/josm:

see #17040 fix typo in ChangesetCacheManager.destroyInstance() so that
ChangesetCacheManagerModel.tearDown() is called

comment:96 Changed 3 weeks ago by GerdP

In 14607/josm:

see #17040 call setTransferHandler(null) in destroy()

comment:97 Changed 3 weeks ago by GerdP

These were my last changes for this year. To be continued...

comment:98 Changed 3 weeks ago by Don-vip

Keywords: memory added
Resolution: fixed
Status: newclosed

We'll open a new ticket then :)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.