Modify

Opened 10 months ago

Closed 9 months ago

Last modified 8 months ago

#15709 closed defect (fixed)

Big image memory leak

Reported by: anonymous Owned by: team
Priority: normal Milestone: 18.01
Component: Core image mapping Version:
Keywords: memory leak ImageTracker ImageMediaEntry ImageDisplay.java Cc:

Description

Since a very long time (maybe 2 years, but it could be much longer), I have a big problem to work with JPEG geo images.
If I do one day of outdoor mapping, I have 1000-2000 JPEG images, but I'm only able to open less than 100 until JOSM fails.
I really hope, that there will by a fix!

JOSM seems to allocate memory for shown images, but does not release the memory.

This is the memory usage in MB (shown by htop):

Before opening the first image:
786

After first image:
973

After second image:
1097

Every line shows the memory usage after opening another image:
1101
1095
1097
1097
1097
1097
1098
1244 (big increase)
1280
1280
1280
1308
1308
1410 (big increase)
1442
1440
1443
1732  (big increase)
1732
1734
1734
1792
1792
1892  (big increase)
1892
1926
1928
1929
1927
2155  (big increase)
2155
2155
2170
2174
2165
2169
2170
2172
2173
2172
2172
2172
2172
2272  (big increase)
2270
2270
2270
2274
2273
2274
2559  (big increase)
2564
2561
2565
2566
2568
2568
2569
2571
2570
2570
2601
2601
2601
2597
2597
2598
2596
2596
2597
2640
2638
2639
2643
2643
2639
2640
2640
2639
2640
2640
2639
2637
2637
2637
2635
2635
2636
2637
2635
2637
2637
2637
2637
2637

JOSM now uses 2639 MB - 786 MB = 1853 MB because I displayed 97 images, after that, JOSM starts to fail to display some images until it is completely out of memory.
I don't think JOSM should use that much memory for images, less than 200 MB should be enough for an endless number of images.

error loading image file
2635
2637
2635
error loading image file
2637
2635
2637
2637
2637
error loading image file
2539
error loading image file
2639
error loading image file
2639
2637
error loading image file
2639
error loading image file
2638
error loading image file
error loading image file

Now JOSM is out of memory:

2017-12-30 12:22:59.912 SCHWERWIEGEND: Handled by bug report queue: java.lang.OutOfMemoryError: Java heap space
java.lang.OutOfMemoryError: Java heap space
        at java.awt.image.DataBufferInt.<init>(DataBufferInt.java:75)
        at java.awt.image.Raster.createPackedRaster(Raster.java:467)
        at java.awt.image.DirectColorModel.createCompatibleWritableRaster(DirectColorModel.java:1032)
        at java.awt.image.BufferedImage.<init>(BufferedImage.java:324)
        at org.openstreetmap.josm.gui.layer.geoimage.ImageDisplay$LoadImageRunnable.run(ImageDisplay.java:328)
        at java.lang.Thread.run(Thread.java:748)

Even if I close the image window and the image layer, JOSM does not free the memory:
2643 MB

I did the whole test, without any data layer, I just opened the images (8 megapixels).

This is what JOSM shows me, after out of memory and after I closed the image window and the image layer:

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-12-28 23:33:18 +0100 (Thu, 28 Dec 2017)
Build-Date:2017-12-29 02:32:16
Revision:13257
Relative:URL: ^/trunk

Identification: JOSM/1.5 (13257 de) Linux Debian GNU/Linux 9.3 (stretch)
Memory Usage: 2389 MB / 2389 MB (71 MB allocated, but free)
Java version: 1.8.0_151-8u151-b12-1~deb9u1-b12, Oracle Corporation, OpenJDK 64-Bit Server VM
Java package: openjdk-8-jre:amd64-8u151-b12-1~deb9u1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-13

Last errors/warnings:
- E: Handled by bug report queue: java.lang.OutOfMemoryError: Java heap space

JOSM is started with "-Xmx2400M".

Attachments (4)

leak.png (28.0 KB) - added by Don-vip 10 months ago.
josm-tracker-early-remove-image.patch (1.4 KB) - added by cmuelle8 10 months ago.
call removeImage(img) right after the MediaTracker has finished loading img
josm_fix-r13264-regression_imageviewerdialog-opening-two-windows-if-undocked_bugfix-variant1.patch (1.5 KB) - added by cmuelle8 10 months ago.
this is almost a revert of r13264, photoadjust plugin will get an Assertion error, as long as geoimage layer has not set up the imageviewer dialog instance
josm_fix-r13264-regression_imageviewerdialog-opening-two-windows-if-undocked_bugfix-variant2.patch (1.7 KB) - added by cmuelle8 10 months ago.
this moves the setup of the image viewer dialog instance to the MapFrame, i.e. to the place where LayerListDialog is set up (and destroyed); if this variant is chosen, it might make sense to refactor LayerListDialog instance creation to match that used in ImageViewerDialog

Download all attachments as: .zip

Change History (64)

comment:1 Changed 10 months ago by Don-vip

In 13263/josm:

see #15709 - fix a major memory leak where the first created ListenerList were instance of ListenerList.TracingListenerList, which keeps a reference to removed listeners, due to bad logging configuration

comment:2 Changed 10 months ago by Don-vip

Keywords: memory added
Milestone: 17.12
Owner: changed from team to anonymous
Status: newneedinfo

Thanks for your report, I have indeed spotted and fixed an obvious memory leak. Can you please try with r13263?

comment:3 Changed 10 months ago by anonymous

Thanks for your fast reply!

I tried with r13263, but it does not help:

Before first image: 885 MB
After first image: 951 MB
After image #50: 2142 MB
At image #85 JOSM starts to fail loading the image: 2635 MB

comment:4 Changed 10 months ago by Don-vip

In 13264/josm:

see #15709 - proper singleton for ImageryViewerDialog

comment:5 Changed 10 months ago by Don-vip

New leak fixed, can you please check with r13264?

Last edited 10 months ago by Don-vip (previous) (diff)

comment:6 Changed 10 months ago by anonymous

r13264 does not help.
Fails after 80 images.

comment:7 Changed 10 months ago by Don-vip

In 13265/josm:

see #15709 - fix a lot of memory leaks. Now gui.layer.geoImage.* classes are correctly garbage collected when the mapframe is destroyed

comment:8 Changed 10 months ago by Don-vip

It should be a lot better with r13265 :)

comment:9 Changed 10 months ago by anonymous

I'm sorry, but r13265 still fails after 80 images.

comment:10 Changed 10 months ago by Don-vip

Yes sorry I didn't work yet on reducing the memory consumption of opening many images, only the memory leaks.
This particular point should be fixed, do you confirm?

Even if I close the image window and the image layer, JOSM does not free the memory

comment:11 Changed 10 months ago by Don-vip

Could you please also share a sample? The gpx track + a hundred of images. I don't have any similar data.

EDIT: forget it I have found an old data set with 175 images and I have no problem: JOSM consumes less than 600Mb and goes down to 200Mb when GC is trigerred.

Can you please share your heap dump? And also the list of plugins you have installed.

Last edited 10 months ago by Don-vip (previous) (diff)

comment:12 Changed 10 months ago by anonymous

This particular point should be fixed, do you confirm?

Even if I close the image window and the image layer, JOSM does not free the memory

No, I can't.
Even if I close the image window and remove the image layer and GPX layer, the memory usage is exactly the same:
htop shows: 2586 MB
JOSM shows: Memory Usage: 2381 MB / 2381 MB (64 MB allocated, but free)

Can you please share your heap dump?

How can I do that?

And also the list of plugins you have installed.

I always test with a new clean profile, so there are no plugins installed.

comment:13 Changed 10 months ago by anonymous

command line:

java -Xmx2400M -Djosm.home=/home/xyz/memory_leak3 -jar ~/josm-latest.jar --language=de

This is the complete status report:

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-12-31 03:09:43 +0100 (Sun, 31 Dec 2017)
Build-Date:2017-12-31 02:33:46
Revision:13265
Relative:URL: ^/trunk

Identification: JOSM/1.5 (13265 de) Linux Debian GNU/Linux 9.3 (stretch)
Memory Usage: 2380 MB / 2380 MB (45 MB allocated, but free)
Java version: 1.8.0_151-8u151-b12-1~deb9u1-b12, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Java package: openjdk-8-jre:amd64-8u151-b12-1~deb9u1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-13
VM arguments: [-Djosm.home=<josm.pref>]
Program arguments: [--language=de]

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Region [TMS_BLOCK_v2] Resetting cache

cache.capabilities1637351842=1514728921
cache.motd.html=1514728921
cache.motd.html.java=1.8.0_151
cache.motd.html.lang=De:
cache.motd.html.version=13265
file-open.history=[/mnt/image_test/2017-12-24.gpx]
geoimage.docked=false
geoimage.geometry=x=-4,y=0,width=1928,height=1048
geoimage.timezone=+0:00
geoimage.visible=false
imagery.entries=[{REMOVED BY ME}]
imagery.layers.default=[Bing, DigitalGlobe-Premium, DigitalGlobe-Standard, EsriWorldImagery, Mapbox, osm-mapnik-black_and_white, standard]
imagery.offsetbookmarks=[]
josm.version=13265
lastDirectory=/mnt/image_test
mappaint.renderer-class-name=org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer
mappaint.style.known-defaults=[resource://styles/standard/elemstyles.mapcss, resource://styles/standard/potlatch2.mapcss]
mirror.https://josm.openstreetmap.de/maps=[1514728925924, <josm.pref>/cache/mirror_https___josm.openstreetmap.de_maps]

comment:14 Changed 10 months ago by Don-vip

OK thanks. You can produce a java heap dump with kill -3 <pid>

EDIT: no sorry that's only for thread dump. Simply add -XX:+HeapDumpOnOutOfMemoryError to your command line and load as many images as you can :)

Last edited 10 months ago by Don-vip (previous) (diff)

comment:15 Changed 10 months ago by Don-vip

Component: CoreCore image mapping

comment:16 in reply to:  14 Changed 10 months ago by anonymous

Replying to Don-vip:

OK thanks. You can produce a java heap dump with kill -3 <pid>

EDIT: no sorry that's only for thread dump. Simply add -XX:+HeapDumpOnOutOfMemoryError to your command line and load as many images as you can :)

Ok, it is dumping now.
But the file gets bigger and bigger.
It would take several days to upload it.

comment:17 Changed 10 months ago by Don-vip

You can compress it of course :)

comment:18 Changed 10 months ago by Don-vip

Once you manage to upload it somewhere, please send me the link privately at vincent at josm.openstreetmap.de.

comment:19 Changed 10 months ago by Don-vip

Milestone: 17.1218.01

comment:20 Changed 10 months ago by cmuelle8

IIRC, while developing on ticket:15574 I've not encountered this. How could I have missed this? If the "Tower of Babel" image had stayed in memory after switching images, I probably would not have had a chance to switch and re-display images.

Maybe the leak is somewhere else and not exactly within ImageDisplay.java and its vicinity?
Or it is jvm / system related and only appears on some configurations.

@Don-vip: You could not reproduce it in comment:11. Do you think you will be able to produce a test case that triggers the leak with the heap dump you requested? It's hard to debug, if we don't know how to reproduce..

Last edited 10 months ago by cmuelle8 (previous) (diff)

comment:21 Changed 10 months ago by Don-vip

with the heap dump I'll see what consumes so much memory, and what is/are the GC root(s) preventing them to be garbage collected. I already fixed all of them I was able to detect on my machine but there was only listeners preventing classes to be garbage collected when the layer is destroyed, not when we switch images.

comment:22 in reply to:  21 ; Changed 10 months ago by cmuelle8

Replying to Don-vip:

listeners preventing classes to be garbage collected when the layer is destroyed, not when we switch images.

That would have only helped OP if he had used a workflow that created and destroyed layers a lot, if I understand correctly?
From the response of OP to the last commits we can deduct that this has not been the case.
Closing leaks not related to this ticket, will still help in the long run, I guess.

comment:23 Changed 10 months ago by anonymous

I will do some investigation.
Memory leak does not occur with all images.
I will try to find the difference, maybe it has something to do with EXIF tags or orientation.

comment:24 Changed 10 months ago by anonymous

Memory leak happens quite fast, if images are in portrait mode, so that exifOrientationSwitchedDimension=true.

I tested the images, generated with:

for i in $(seq 1 400); do convert -size 4128x3096 xc:white jpeg:- | exiftool -Orientation#=6 -ImageHeight=3096 -ImageWidth=4128 -GPSLongitudeRef=E -GPSLongitude=120.$(expr 100 + $i) -GPSLatitudeRef=N -GPSLatitude=30.$(expr 200 + $i) - -o image_$i.jpg; done

If I don't set "-Orientation#=6" there seems to be no memory leak or maybe it occurs very slowly, I gave up after 200 images...

Maybe you can reproduce?

I did not set any Java parameters (no Xmx).

Images start to fail loading quite fast, but OOM often does not occur, I think this is because of mayFitMemory().

comment:25 Changed 10 months ago by anonymous

With "-Orientation#=6" images I get OOM after 49 images.

comment:26 Changed 10 months ago by anonymous

counter-check without "-Orientation#=6" (so exifOrientationSwitchedDimension=false):
NO OOM after 400 landscape format images!

Thus I think, it may have to do something with:

                    if (ExifReader.orientationNeedsCorrection(entry.getExifOrientation())) {
                        if (ExifReader.orientationSwitchesDimensions(entry.getExifOrientation())) {
                            width = img.getHeight(null);
                            height = img.getWidth(null);
                            switchedDim = true;
                        }
                        final BufferedImage rot = new BufferedImage(width, height, BufferedImage.TYPE_INT_RGB);
                        final AffineTransform xform = ExifReader.getRestoreOrientationTransform(
                                entry.getExifOrientation(),
                                img.getWidth(null),
                                img.getHeight(null));
                        final Graphics2D g = rot.createGraphics();
                        g.drawImage(img, xform, null);
                        g.dispose();
                        img.flush();
                        img = rot;
                    }
Last edited 10 months ago by Don-vip (previous) (diff)

comment:27 in reply to:  22 Changed 10 months ago by Don-vip

Replying to cmuelle8:

Closing leaks not related to this ticket, will still help in the long run, I guess.

Oh yes. That's the problem with memory leaks, you can't solve a problem without resolving them all. Basically when you destroy the map frame, JOSM is supposed to go back almost to its initial state in terms of memory usage.

comment:28 in reply to:  26 ; Changed 10 months ago by cmuelle8

Replying to anonym:

Thus I think, it may have to do something with..

The snippet from comment:26 has not been around for two years in that specific form, but you write that you are experiencing the problem since then. Thus, if the leak is within that code block, it must have been in its predecessor(s) as well:

no changes within the snippet in the latest two file revisions:

no changes within the snippet in these previous four file revisions:

no changes within the snippet in these previous eight file revisions:

r9079 is about 2 years old, r13038 just 2 months.

Can you reproduce the behavior you have observed in
comment:26 with a version between r9079 and r13038?


r13127 to r13186 (inclusive) did not switch dimensions correctly
in the event an image was rotated due to the exif orientation tag,
this was fixed in r13191, see ticket:15625. Regarding the leak
discussed here these revisions should exhibit the same behavior,
presumably.

comment:29 in reply to:  24 ; Changed 10 months ago by Don-vip

Replying to anonyme:

I tested the images, generated with:

for i in $(seq 1 400); do convert -size 4128x3096 xc:white jpeg:- | exiftool -Orientation#=6 -ImageHeight=3096 -ImageWidth=4128 -GPSLongitudeRef=E -GPSLongitude=120.$(expr 100 + $i) -GPSLatitudeRef=N -GPSLatitude=30.$(expr 200 + $i) - -o image_$i.jpg; done

Can you please share these images? The command doesn't work for me, I get "Error: Can't create JPEG files from scratch"

comment:30 in reply to:  29 ; Changed 10 months ago by anonymous

Replying to Don-vip:

Replying to anonyme:

I tested the images, generated with:

for i in $(seq 1 400); do convert -size 4128x3096 xc:white jpeg:- | exiftool -Orientation#=6 -ImageHeight=3096 -ImageWidth=4128 -GPSLongitudeRef=E -GPSLongitude=120.$(expr 100 + $i) -GPSLatitudeRef=N -GPSLatitude=30.$(expr 200 + $i) - -o image_$i.jpg; done

Can you please share these images? The command doesn't work for me, I get "Error: Can't create JPEG files from scratch"

Here is the upload:
https://we.tl/rWIDoodACl

comment:31 in reply to:  28 Changed 10 months ago by anonymous

Replying to cmuelle8:

Can you reproduce the behavior you have observed in
comment:26 with a version between r9079 and r13038?

Not reproducible with r9079, JOSM shows:

Memory Usage: 858 MB / 858 MB (39 MB allocated, but free)

The last value "allocated, but free" jumps up and down, the first value stays at 858.

comment:32 in reply to:  30 ; Changed 10 months ago by Don-vip

Replying to anonyme:

Here is the upload:
https://we.tl/rWIDoodACl

Thanks. For me, the 400 images load in a matter of seconds and JOSM only consumes 700Mb. Please share your (compressed) heapdump.

comment:33 in reply to:  32 Changed 10 months ago by anonymous

Replying to Don-vip:

For me, the 400 images load in a matter of seconds and JOSM only consumes 700Mb.

Did you display all 400 images (for example via click on the "next" button), one by one?
Just to make sure, that we are talking about the same issue. :)

comment:34 Changed 10 months ago by Don-vip

I didn't. OK now I observe a memory leak.

comment:35 Changed 10 months ago by cmuelle8

I have confirmed it as well, but looking at the code snippet in comment:26, I'd say the leak is somewhere else. That snippet even calls img.flush(); to free the resources held by img, before img = rot;, the point at which the old reference img was pointing to is supposedly ready to be garbage collected.

It may be that there is yet something overlooked, but judging from a first review, I'd say that snippet is ok. I'll try with 13038 and see if it happens there. Bisecting the version it appears first will surely help.

comment:36 in reply to:  35 Changed 10 months ago by anonymous

Replying to cmuelle8:

I have confirmed it as well, but looking at the code snippet in comment:26, I'd say the leak is somewhere else. That snippet even calls img.flush(); to free the resources held by img, before img = rot;, the point at which the old reference img was pointing to is supposedly ready to be garbage collected.

It may be that there is yet something overlooked, but judging from a first review, I'd say that snippet is ok.

Yes, now I also think, the snippet is ok.

I just tried VisualVM:
"biggest objects" seems to show that MediaTracker and ImageMediaEntry are huge, after many images have been displayed?

But I don't know anything about how to use and interpret VisualVM...

comment:37 Changed 10 months ago by Don-vip

Owner: changed from anonymous to team
Status: needinfonew

Changed 10 months ago by Don-vip

Attachment: leak.png added

comment:38 Changed 10 months ago by Don-vip

ImageMediaEntry are chained. every instance keeps track of the following one, thus creating the leak:


comment:39 Changed 10 months ago by cmuelle8

It's definitely not around for two years, so far I've tested:

r12618, r13035, r13038, r13128, r13136, r13186 negative

and r13197 positive

comment:40 Changed 10 months ago by Don-vip

Probable regression of r13191 with the use of tracker.addImage

comment:41 Changed 10 months ago by Don-vip

In 13270/josm:

see #15709 - fix a memory leak found during investigation

comment:42 Changed 10 months ago by anonymous

I added this line, it seems to help:

g.drawImage(img, xform, null);
g.dispose();
tracker.removeImage(img);
img.flush();
img = rot;

But it was just for fun and testing purposes, so it is likely not a correct fix. :)

comment:43 in reply to:  40 Changed 10 months ago by anonymous

Replying to Don-vip:

Probable regression of r13191 with the use of tracker.addImage

that was in the code before, but it seems anonym found the issue..

EDIT: this comment was originally added and later edited by cmuelle8
to acknowledge the solution of OP in comment:42 (seems important to
note here, since OP is using anonym alias exclusively to comment)

Last edited 10 months ago by cmuelle8 (previous) (diff)

comment:44 Changed 10 months ago by cmuelle8

tracker.removeImage(img); is called after the code block, but it will be called on the wrong ref (rot) in case the image was rotated due to the exif rotation flag.

the only question remaining is if you should call it before img.flush(); or thereafter..

comment:45 Changed 10 months ago by cmuelle8

actually img.flush(); should be removed!!

it is not called in the landscape code path, is it? and we have not had problems there..

it just seems important that it's correctly removed from the tracker..

the doc to flush(); says that its normally not needed,
and if we do, we should call it in the landscape code path as well.

comment:46 in reply to:  39 ; Changed 10 months ago by anonymous

Replying to cmuelle8:

It's definitely not around for two years, so far I've tested:

I guess, I misinterpreted the OOM while displaying images and the cause was another memory leak (caused by a plugin, background image layer or something else)...
Sadly, I did never before a clean test and without any other layers.

comment:47 Changed 10 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 13271/josm:

fix #15709 - fix memory leak when browsing geoimages: remove correct image reference from media tracker after rotation (regression from r13191)

comment:48 in reply to:  46 Changed 10 months ago by cmuelle8

Replying to anonym:

Sadly, I did never before a clean test and without any other layers.

no hard feelings, we still found a bug, after all :)

Before r13191 img was not reassigned, ImageDisplay.this.image = rot; was used right away in this code block.

I changed this to achieve more atomicity in the rotating case, and to use a single assignment to ImageDisplay.this.image, but overlooked that the MediaTracker would cling on to a ref, if it did not receive a removeImage() instruction.

The reason to reassigning img is just imho: If there ever is a need to do more post-processing to img in the future, we now have the modularity to plug in a code section in between the rotating code and the finalizing assignment to ImageDisplay.this.image in this method.

Last edited 10 months ago by cmuelle8 (previous) (diff)

comment:49 Changed 10 months ago by Don-vip

Thanks to both of you for the investigation :) The good news is that I have fixed a lot of unrelated memory leaks in the course of this ticket, so JOSM should be less memory hungry.

comment:50 in reply to:  46 ; Changed 10 months ago by cmuelle8

Replying to anonym:

I misinterpreted the OOM while displaying images and the cause was another memory leak (caused by a plugin, background image layer or something else)...

If this means you originally posted this bug to bring attention to another leak, you should open a new bug (or reopen this one). If there really is a long standing bug that exists for about two years, then it will likely persist and not have been fixed by r13271.

EDIT: The persisting problem may be unrelated to josm-core (and exif rotation state) and indeed backtrace to one of the plugins, but that would have to be researched anew, if you can still confirm a leak issue within the image viewing components.

Last edited 10 months ago by cmuelle8 (previous) (diff)

comment:51 in reply to:  50 Changed 10 months ago by anonymous

Replying to cmuelle8:

If this means you originally posted this bug to bring attention to another leak, you should open a new bug (or reopen this one). If there really is a long standing bug that exists for about two years, then it will likely persist and not have been fixed by r13271.

If I find another memory leak, I will report it. :)

comment:52 Changed 10 months ago by cmuelle8

Keywords: leak ImageTracker ImageMediaEntry ImageDisplay.java added
Resolution: fixed
Status: closedreopened

Within the course of fixing this ticket, an regression was introduced.

If the image viewer is undocked, then after opening a fresh instance of josm and loading images, the viewer panel is opened twice, but only one instance has content. Closing one of these two undocked windows, closes both. Reopening the viewer without having closed josm, the behavior does not repeat (i.e. reopening is ok).

I have checked r13263 to be ok, but r13264, r13265 and onward have the regression.

EDIT: The second (empty, undocked) viewer panel on opening images is only shown with the photoadjust plugin enabled.


Unrelated to this, I have looked into the MediaTracker.removeImage() methods a little further, and it seems ok, if we remove the img reference from the MediaTracker a lot earlier (i.e. after loading has finished). Please find the patch attached.

Last edited 10 months ago by cmuelle8 (previous) (diff)

Changed 10 months ago by cmuelle8

call removeImage(img) right after the MediaTracker has finished loading img

comment:53 Changed 10 months ago by cmuelle8

If (solely) changeset:13264 is reverted, a single undocked window is shown as expected.

@Don-vip: Can you explain what leak r13264 fixes?


Here is a log from josm start that shows that getInstance() method of the ImageViewerDialog is called quite often on image loading.

2018-01-02 03:11:08.891 INFO: Open 400 files
ImageViewerDialog.getInstance() called.
ImageViewerDialog.getInstance() called.
ImageViewerDialog.getInstance() called.
ImageViewerDialog.getInstance() called.
2018-01-02 03:11:10.222 INFO: Loading /home/user/wetransfer-e30e13/image_1.jpg using default toolkit
ImageViewerDialog.getInstance() called.
ImageViewerDialog.getInstance() called.
2018-01-02 03:11:10.454 INFO: Loaded /home/user/wetransfer-e30e13/image_1.jpg with dimensions 3,096x4,128 memoryTaken=48m exifOrientationSwitchedDimension=true

According to https://www.javamex.com/tutorials/synchronization_volatile.shtml volatiles should be self synchronizing after java 5, so r13264 should work, considering that getInstance() may be called from different threads, shouldn't it?

EDIT: The LayerListDialog creates its instance in a separate static method as well, there must be a reason to do so. Prior to r13264 ImageViewerDialog used that same creation pattern. I'm rather curious, why r13264 truely makes a difference and exhibits unexpected behavior when combined with an enabled photoadjust plugin.

EDIT2: The photoadjust plugin calls ImageViewerDialog.getInstance() earlier than built-in GeoImageLayer.java does, but it probably should not get a valid instance before GeoImageLayer had a chance to do addToggleDialog() for the instance (at least, prior to r13264 it did get the AssertionError until after GeoImageLayer.java (!) specifically created the instance). So r13264 changed creation time and creator of the ImageViewerDialog instance.

Last edited 10 months ago by cmuelle8 (previous) (diff)

Changed 10 months ago by cmuelle8

this is almost a revert of r13264, photoadjust plugin will get an Assertion error, as long as geoimage layer has not set up the imageviewer dialog instance

Changed 10 months ago by cmuelle8

this moves the setup of the image viewer dialog instance to the MapFrame, i.e. to the place where LayerListDialog is set up (and destroyed); if this variant is chosen, it might make sense to refactor LayerListDialog instance creation to match that used in ImageViewerDialog

comment:54 Changed 10 months ago by cmuelle8

If variant2 of the fix is used, the photoadjust plugin also gets a valid instance at plugin loading time before the map frame is created (like described in EDIT2 of comment:53). But moving addToggleDialog(ImageViewerDialog.getInstance()); from GeoimageLayer.java to MapFrame.java resolves the double-undocked-dialog-open bug as well.

So creation time and creator of the image viewer dialog instance seems to be of importance when addToggleDialog is called from GeoimageLayer, but less so when called from MapFrame.

However, if we adhere to lazy loading principle, we should not create the image viewer dialog instance at plugin loading time. Especially not so, if the plugin (photoadjust) does not operate without a valid geoimage layer opened.

@Don-vip: To restore lazy loading, I'd opt to resort to variant1, but you may find better reasons to go with variant2.

comment:55 Changed 9 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 13289/josm:

fix #15709 - revert r13264 to restore proper lazy initialization of ImageViewerDialog (patch by cmuelle8)

comment:56 Changed 9 months ago by cmuelle8

Resolution: fixed
Status: closedreopened

Please also apply remaining attachment:josm-tracker-early-remove-image.patch, it will help future patch authors to not run into the regression dealt with in comment:47 again. This has practical relevance if the remnants of ticket:15574 ever gain broader audience, say if the scope changes that currently limits its usefulness / applicability.

I've tested it ok, using the image set OP supplied and in day to day usage without a problem.

OT: Btw, is this feature of jenkins disabled on purpose or just because noone has noticed it? The downloads folder of the website and the -latest / -tested symlinks could instead point to the locations jenkins archives the builds at (if hosting duplicates is not desirable), couldn't they?

Last edited 9 months ago by cmuelle8 (previous) (diff)

comment:57 Changed 9 months ago by Don-vip

Official JOSM binaries are not produced by Jenkins (because the build infrastructure is older, and there's no need to sign Jenkins artifacts).

comment:58 Changed 9 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 13295/josm:

fix #15709 - apply last patch of cmuelle8

comment:59 Changed 9 months ago by Don-vip

In 13327/josm:

see #15709 - sonar - squid:S2387 - "mouseListener" is the name of a field in "Component"

comment:60 Changed 8 months ago by holgermappt

The photoadjust plugin just needs to know if there is an image displayed in the ImageViewerDialog. Would it make sense to add a method that can be used to get that information? Right now the getInstance() call is used in the photoadjust plugin because that method exists and can be used to get the required information (with a catched exception). Using a more specific method would allow to refactor the code without the seen side effects.

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.