Modify

Opened 7 days ago

Closed 78 minutes ago

#19789 closed defect (fixed)

[RFC patch] memory leak with gpx waypoints

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: 20.09
Component: Core Version:
Keywords: template_report performance gpx Cc: simon04, taylor.smock

Description

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)

Attachments (2)

19789-v0.patch (2.1 KB) - added by GerdP 7 days ago.
Patch (alpha) to implement the removal of listeners.
19789-v1.patch (3.3 KB) - added by GerdP 6 days ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 days ago by GerdP

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

Changed 7 days ago by GerdP

Attachment: 19789-v0.patch added

Patch (alpha) to implement the removal of listeners.

comment:2 Changed 7 days ago by GerdP

Owner: changed from team to GerdP
Status: newassigned
Summary: memory leak with gpx waypoints[RFC patch] memory leak with gpx waypoints

comment:3 Changed 7 days ago by GerdP

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

comment:4 in reply to:  2 Changed 7 days ago by skyper

Replying to GerdP:
Welcome back from holidays.

comment:5 Changed 7 days ago by Klumbumbus

How was the cycling?

comment:6 Changed 7 days ago by GerdP

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

comment:7 Changed 7 days ago by Klumbumbus

Uh, thats a lot :o

Changed 6 days ago by GerdP

Attachment: 19789-v1.patch added

comment:8 Changed 6 days ago by GerdP

Cc: simon04 taylor.smock added
Milestone: 20.09

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?

comment:9 in reply to:  8 Changed 6 days ago by 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.

comment:10 Changed 78 minutes ago by GerdP

Resolution: fixed
Status: assignedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.