[Patch] Reduce memory footprint in Node and WayPoint

[anonymous]

The attached patch reduces the memory footprint of the Node and WayPoint classes. This is relevant, because JOSM internally creates a lot of Node instances (in particular in large datasets) and a lot of WayPoints, if GPS-Data is downloaded from the OSM server.

The patch mainly:

• makes CachedLatLon obsolete. Major drawback of current CachedLatLon: each instance keeps a reference to the current global projection
• inlines the lat/lon coordinates and the cached, projected east/north coordinates in the classes Node and WayPoint
• properly reprojects Nodes and WayPoints, when the projection is changed

See ​this branch with one combined commit on GitHub, or ​this brach with individual smaller commits.

Attached a patch against JOSM 4114, formatted using git format-patch. If applied in Eclipse, you'll have to manually merge the very last entry. Just accept the update to Main.setProjection(new Epsg4326()); and ignore the rest.

[anonymous]

Patch

[anonymous]

Caveat: if applied, this patch will probably break some plugins.

Leave a comment if you need help on checking and fixing them.

[stoecker]

Looks good on first view.

Now only proper handling of projections and projection changes for imagery layers is missing.

[jttt]

I think it's a bit error-prone. If Node is not part of the Dataset and Dataset part of OsmDataLayer then projected coordinates will not get updated. Maybe Node.getEastNorth should recalculate coordinates every time if it has no Dataset assigned.

Dataset without OsmDataLayer - only way I can think of is having Dataset directly registered as ProjectionChangedListener (via SoftReference to prevent memory leaks).

[anonymous]

Updated patch (forgot to remove one premature optimization in NodeData)

[bastiK]

Only a small fraction of users will switch projections regularly, so errors might creep in and stay undetected. But I guess it's worth it. Some minor comments:

• Why is CachedLatLon "uncached"? Isn't it enough to deprecate the class?
• The projected coordinates could be calculated "on demand". (Just a matter of taste, imagine JOSM would run in headless mode and could be used as a tool like osmosis. Then the projecting would be unnecessary.)
• Patch does not include ProjectionChangeListener. The file can be found here: ​https://github.com/Gubaer/josm/blob/ba946d1b88d913e33b7cbbc3118391e4993d0b52/src/org/openstreetmap/josm/data/projection/ProjectionChangeListener.java

[anonymous]

Replying to jttt:

I think it's a bit error-prone. If Node is not part of the Dataset and Dataset part of OsmDataLayer then projected coordinates will not get updated. Maybe Node.getEastNorth should recalculate coordinates every time if it has no Dataset assigned.

Dataset without OsmDataLayer - only way I can think of is having Dataset directly registered as ProjectionChangedListener (via SoftReference to prevent memory leaks).

Thanks, jttt. See last three commits in ​https://github.com/Gubaer/josm/commits/cached-lat-lon

[anonymous]

• Why is CachedLatLon "uncached"? Isn't it enough to deprecate the class?

Yes.

• The projected coordinates could be calculated "on demand". (Just a matter of taste, imagine JOSM would run in headless mode and could be used as a tool like osmosis. Then the projecting would be unnecessary.)

JOSM in headless mode? Hmm, personally I rather doubt about it. But calculating coordinates on demand? Yes, why not.

• Patch does not include ProjectionChangeListener. The file can be found here: ​https://github.com/Gubaer/josm/blob/ba946d1b88d913e33b7cbbc3118391e4993d0b52/src/org/openstreetmap/josm/data/projection/ProjectionChangeListener.java

Yes. Later I'm going to supply a new version of the patch, including the latest modifications and including ProjectionChangeListener

[bastiK]

Replying to anonymous:

• The projected coordinates could be calculated "on demand". (Just a matter of taste, imagine JOSM would run in headless mode and could be used as a tool like osmosis. Then the projecting would be unnecessary.)

JOSM in headless mode? Hmm, personally I rather doubt about it. But calculating coordinates on demand? Yes, why not.

Off topic: There could be command line rendering like that: java -jar josm-tested.jar --render --input city.osm --style test.mapcss --scale 20000 --bounds ... --output city.png. This still projects all the Nodes, but it would be a start for headless mode.

[anonymous]

Replying to bastiK:

Replying to anonymous:

• The projected coordinates could be calculated "on demand". (Just a matter of taste, imagine JOSM would run in headless mode and could be used as a tool like osmosis. Then the projecting would be unnecessary.)

JOSM in headless mode? Hmm, personally I rather doubt about it. But calculating coordinates on demand? Yes, why not.

Off topic: There could be command line rendering like that: java -jar josm-tested.jar --render --input city.osm --style test.mapcss --scale 20000 --bounds ... --output city.png. This still projects all the Nodes, but it would be a start for headless mode.

I onced tried to do offline rendering in order to performance test MapCSS rendering, see ​https://github.com/Gubaer/josm/blob/mirror/test/functional/mapcss/performance/PerformanceTest.groovy

I was able to render into an image by supplying a custom Grapics2D context, but I nevertheless neded a MapView because it is currently tightly coupled with map rendering. That's why, I doubt, that JOSM is currently ready for headless rendering. But I have to admit that I didn't spend a lot of time with this, I can easily have overlooked something.

[stoecker]

The CachedLatLon was introduced by me a long time ago, as there were performance problems with permanent coordinate calculations. Since then a lot in the infrastructure has changed, so we should really test if "on demand" is possible now again. If would certainly make the whole stuff a lot easier (I never liked CachedLatLon at all).

But drawing must not get slower.

But this only applies to the data we actually draw. Probably we can calculate these values on demand for everything which does not match the "in an osm data layer" assumption. This may help to get complexity in balance with execution speed.

[bastiK]

Replying to stoecker:

The CachedLatLon was introduced by me a long time ago, as there were performance problems with permanent coordinate calculations. Since then a lot in the infrastructure has changed, so we should really test if "on demand" is possible now again. If would certainly make the whole stuff a lot easier (I never liked CachedLatLon at all).

What I mean by "on demand", is to initialize the cache with NaN and keep it this way until EastNorth is requested for the first time. For subsequent calls, the cached value is returned. (Lazy evaluation)

But drawing must not get slower.
But this only applies to the data we actually draw. Probably we can calculate these values on demand for everything which does not match the "in an osm data layer" assumption. This may help to get complexity in balance with execution speed.

Agreed.

[anonymous]

What I mean by "on demand", is to initialize the cache with NaN and keep it this way until EastNorth is requested for the first time. For subsequent calls, the cached value is returned. (Lazy evaluation)

Me too, see ​https://github.com/Gubaer/josm/commits/cached-lat-lon

[stoecker]

In last change: I would drop the "if (Main.getProjection() == null) return; // sanity check" in the invalidate function.

[stoecker]

@bastik: Do we want to make a release first or delay it and apply this now (we had a lot of API changes already, so I doubt the plugins are all up-to-date anyway)?

[bastiK]

Replying to stoecker:

@bastik: Do we want to make a release first or delay it and apply this now (we had a lot of API changes already, so I doubt the plugins are all up-to-date anyway)?

Doesn't matter to me, both is possible. In case of delayed release, we could include the parallel way feature, as well.

[bastiK]

Replying to anonymous:

Replying to bastiK:

Replying to anonymous:

• The projected coordinates could be calculated "on demand". (Just a matter of taste, imagine JOSM would run in headless mode and could be used as a tool like osmosis. Then the projecting would be unnecessary.)

JOSM in headless mode? Hmm, personally I rather doubt about it. But calculating coordinates on demand? Yes, why not.

Off topic: There could be command line rendering like that: java -jar josm-tested.jar --render --input city.osm --style test.mapcss --scale 20000 --bounds ... --output city.png. This still projects all the Nodes, but it would be a start for headless mode.

I onced tried to do offline rendering in order to performance test MapCSS rendering, see ​https://github.com/Gubaer/josm/blob/mirror/test/functional/mapcss/performance/PerformanceTest.groovy

I was able to render into an image by supplying a custom Grapics2D context, but I nevertheless neded a MapView because it is currently tightly coupled with map rendering. That's why, I doubt, that JOSM is currently ready for headless rendering. But I have to admit that I didn't spend a lot of time with this, I can easily have overlooked something.

I think you are right, but that doesn't mean it can't be fixed.

[anonymous]

Updated patch against JOSM 4115, includes suggestions from this ticket

[stoecker]

1) Please remove CachedLatLon totally:

• it is only used by editgpx plugin
• it would be dropped from released .jar anyway, as it is no longer used in core

2) Setting a SubPrefs projection fails:

• If I try UTM zone 33 in prefs, I get UTM zone 30 as result in the mapview.

3) A minor issue: GpxLayer misses a return at file end.

[anonymous]

I won't supply another patch. 1) and 3) can be fixed by anybody in less than five minutes. Don't try to keep me busy.

Ad 2): Try to replace ​this with

if (p instanceof ProjectionSubPrefs) {
   ((ProjectionSubPrefs) p).setPreferences(coll);
}

[stoecker]

Sure 1 and 3 are trivial and I already did these. 2 was the reason why I did not apply the patch yet.

But your answer again gives me doubt whether to apply it at all. This was the problem with your changes in the past and it seems it still is - you make great adaptions, but are not willing to fix all the little quirks related to these. I don't have enough time to do this myself so I must rely on the original patch authors.

[anonymous]

Closing. Not worth the trouble. Please don't apply the patch.

[anonymous]

In [4126] integrated by bastik. Thanks.