[RFC patch] memory leak with gpx waypoints

[GerdP]

What steps will reproduce the problem?

1. load a gpx file with many waypoints (wpt)
2. remove the new layers created by step 1
3. repeat 1+2

What is the expected result?

Each repetition should show roughly the same load time

What happens instead?

load times are increasing

2020-09-13 14:47:38.878 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes)
2020-09-13 14:47:41.853 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes) completed in 3.0 s
2020-09-13 14:49:16.120 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes)
2020-09-13 14:49:24.141 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes) completed in 8.0 s
2020-09-13 14:49:56.095 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes)
2020-09-13 14:50:09.118 INFO: Open file: D:\mkgmap\land_hp.gpx (1489503 bytes) completed in 13.0 s

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

Problem is in class Marker which adds a listener for each way point (wpt):

Preferences.main().addKeyPreferenceChangeListener("draw.rawgps." + getTextTemplateKey(), l -> updateText());

These listeners are not removed when the marker layer is closed. So, this eats up memory and the method addListener() calls ensureNotInList() which does a sequential search over all listeners. My test file contains > 32.000 points

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-09-06 16:54:59 +0200 (Sun, 06 Sep 2020)
Build-Date:2020-09-07 01:30:48
Revision:17013
Relative:URL: ^/trunk

Identification: JOSM/1.5 (17013 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2004 (19041)
Memory Usage: 895 MB / 3641 MB (200 MB allocated, but free)
Java version: 1.8.0_221-b11, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920x1080 (scaling 1.0x1.0)
Maximum Screen Size: 1920x1080
Best cursor sizes: 16x16 -> 32x32, 32x32 -> 32x32
VM arguments: [-XX:StartFlightRecording=name=MyRecording2,settings=d:\dbg\gerd.jfc, -XX:FlightRecorderOptions=defaultrecording=true,dumponexit=true,dumponexitpath=e:\ld\perf_20200913_144555.jfr]

Plugins:
+ OpeningHoursEditor (35414)
+ PolygonCutOut (v0.7)
+ apache-commons (35524)
+ buildings_tools (35500)
+ continuosDownload (91)
+ ejml (35313)
+ geotools (35169)
+ jaxb (35092)
+ jts (35122)
+ merge-overlap (35248)
+ o5m (35248)
+ opendata (35513)
+ pbf (35446)
+ poly (35248)
+ reverter (35499)
+ undelete (35521)
+ utilsplugin2 (35487)

[GerdP]

The memory leak also includes the WayPoint class. Seems the GpxLayer instances are not destroyed properly.

[GerdP]

Patch (alpha) to implement the removal of listeners.

[GerdP]

I wonder if we really need a listener in each Marker instance. Wouldn't it be better to have that listener in MarkerLayer?

[skyper]

Replying to GerdP:
Welcome back from holidays.

[Klumbumbus]

How was the cycling?

[GerdP]

It was a nice roundtrip through Germany and Austria. 3200 km in 31 days, only one day with bad weather :)

[Klumbumbus]

Uh, thats a lot :o

[GerdP]

Reg. comment:3:
It would require lots of changes to implement this in MarkerLayer as the field data is public and it is used in some plugins. I guess that's the reason for the implementation in Marker.
Patch 19789-v1.patch addresses also the WayPoint instances.

• implement destroy() in Marker
• add code in AutosaveTask to handle removal of AbstractModifiableLayer (regression from r16548 for #18801)

Please review:

• I am not sure regarding the static field cachedTemplates in Marker. Why didn't method updateText() use clear() before?
• I am not familiar with the code in AutosaveTask. Was there a reason to not handlle the removal in r16548?

[taylor.smock]

Replying to GerdP:

• I am not familiar with the code in AutosaveTask. Was there a reason to not handlle the removal in r16548?

In that patch, I was trying to keep total changed functionality to a minimum, so I only checked the specific cases that were already handled. I also didn't touch the layer removal code. I should have added the AbstractModifiableLayer to the layer removal code at the time, but I didn't think about it.

[GerdP]

In 17047/josm:

fix #19789: memory leak with gpx waypoints

• implement destroy in class Marker and call it for each object in a destroyed MarkerLayer
• handle also AbstractModifiableLayer in AutosaveTask