Modify

Opened 3 months ago

Closed 5 weeks ago

Last modified 3 weeks ago

#18907 closed enhancement (fixed)

[Patch] Initialize Territories+RightAndLefthandTraffic together, remove "Edit boundaries", save memory

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 20.05
Component: Core Version:
Keywords: performance heap yourkit Cc: Don-vip

Description

Can we remove "Edit boundaries" from the advanced preferences? If yes, we could get rid of the DataSet from boundaries.osm on the heap after initialization and spare ≈700kB of memory.

org.openstreetmap.josm.data.osm.DataSet#1		230 B (0%)	658,890

Attachments (1)

18907.patch (15.3 KB) - added by simon04 3 months ago.

Download all attachments as: .zip

Change History (19)

Changed 3 months ago by simon04

Attachment: 18907.patch added

comment:1 in reply to:  description Changed 3 months ago by Don-vip

Replying to simon04:

Can we remove "Edit boundaries" from the advanced preferences?

Do you see a way to maintain the file without it? I didn't.

comment:2 Changed 3 months ago by Don-vip

You can make it an advanced preference disabled by default, but I need to be able to edit the file.

comment:3 Changed 3 months ago by simon04

I just opened boundaries.osm from the checked out JOSM source code. I wasn't aware of this feature until today.

comment:4 in reply to:  3 Changed 3 months ago by Don-vip

Replying to simon04:

I just opened boundaries.osm from the checked out JOSM source code.

This way all ids will change when you save the file back, so it doesn't work. Another option would be to add a command-line argument disabling the initialization of territories, so we could open the file from the filesystem, at the loss of some functionalities.

comment:5 Changed 2 months ago by simon04

Milestone: 20.0320.04

comment:6 Changed 7 weeks ago by simon04

Resolution: fixed
Status: assignedclosed

In 16321/josm:

fix #18907 - Initialize Territories+RightAndLefthandTraffic together, remove "Edit boundaries" to save memory (unless started with --debug)

comment:7 Changed 7 weeks ago by simon04

Keywords: yourkit added

comment:9 Changed 6 weeks ago by taylor.smock

@simon04: Is there any chance we can make modifications to the class so that we can get information that is removed (using the KNOWN_KEYS list)? I.e., something like Territories.getAllTags(String code). I know you made this change to save memory, and I don't know if keeping the tags in the KNOWN_KEYS list will make the change counter productive.

I had been using it to map names that MapWithAI was giving me to locations (so Alaska -> US-AL). Since Territories now sets the DataSet to null, if I call Territories.getOriginalDataSet, I get a NPE (I had been using Territories.getDataSet, but that was removed in the same changeset that defaults to setting the Territories dataset to null).

I had (essentially) been doing Territories.getOriginalDataSet().allPrimitives().parallelStream().filter(o -> o.hasKey("name:en") && o.get("name:en").equalsIgnoreCase(countryName)).findAny() to take a country name and map it to a location.

If all else fails, I can (manually) make modifications to an (out of date) "copy" I made in the MapWithAI repo. It will probably be out of date every so often though. :(

Note: The plugin reads https://github.com/facebookmicrosites/Open-Mapping-At-Facebook/blob/master/data/rapid_releases.geojson for data.

comment:10 Changed 6 weeks ago by simon04

Resolution: fixed
Status: closedreopened

Hi taylor.smock, sorry for breaking your plugin. I think we can spare the memory for 435 names of countries/states (previously we've been keeping the metadata of hundreds of nodes). Would you provide a patch for the relevant changes best fitting your needs?

comment:11 in reply to:  10 Changed 6 weeks ago by taylor.smock

Replying to simon04:

Hi taylor.smock, sorry for breaking your plugin. I think we can spare the memory for 435 names of countries/states (previously we've been keeping the metadata of hundreds of nodes). Would you provide a patch for the relevant changes best fitting your needs?

Don't worry (too much) about breaking my plugin -- you couldn't have known I had been using the Territories Dataset. I know that JOSM core can really only care about the plugins that the JOSM core devs can fix/make modifications to. Its one of the reasons I want to move my plugins into the JOSM organization sometime, but I'm waiting on the move to gitlab with #16857.

I can go ahead and make a patch if it is still desired. That being said, I no longer need it -- I didn't get a reply fast enough, and the next milestone was due soon, so I took the time to update my (naive) copy of the data and rip out the code that took the originating rapid_releases.geojson and converted it into a format usable for the plugin.

Since I no longer need the functionality, do you think it would be worth it to keep the data anyway? (ISO3166_1, ISO3166_2, TAGINFO, "type", "name:en", "driving_side", "note") Probably the big ones to keep would be the ISO3166 and name:en tags.

comment:12 Changed 6 weeks ago by simon04

Most of the time I check for usages in plugins before changing an API, sometimes I forget, though.

The ISO3166 codes go into the keys of Map<String, TagMap> customTagsCache, so I don't know, whether we should keep them? The name:en seem to be of interest.

Thus, I'll drop "name:en" from KNOWN_KEYS?

comment:13 Changed 6 weeks ago by taylor.smock

That is probably the easiest and simplest solution. It also doesn't have as much overhead as creating an entirely new variable to hold them.

Something to keep in mind is that the customTagsCache currently has very few entries relative to the number of countries in the world. The only profiler I have access to is VisualVM, and that doesn't really tell me what is using all the memory (it might say int[] uses 133MB, but it doesn't break down what classes use it), so I have no idea how that will affect memory usage.

Again, as far as checking for usages in plugins before changing an API, the MapWithAI plugin isn't cloned as part of the JOSM source tree (when you get the plugins as well), so I would not expect you (or anyone else) to know that you broke the plugin.

Last edited 6 weeks ago by taylor.smock (previous) (diff)

comment:14 Changed 6 weeks ago by simon04

Retained size of customTagsCache increases from 4_120 to 38_112 when including "name:en". Still a viable amount, I'd say.

comment:15 Changed 6 weeks ago by taylor.smock

So the net memory gain should be 658_890 - (38_112 - 4_120), or 624_898 if name:en is kept. I guess 625 KB is a win. :)

comment:16 Changed 6 weeks ago by simon04

In 16381/josm:

see #18907 - Territories: retain "name:en" for custom tags

comment:17 Changed 5 weeks ago by simon04

Resolution: fixed
Status: reopenedclosed

comment:18 Changed 3 weeks ago by Klumbumbus

Milestone: 20.0420.05

Milestone renamed

Modify Ticket

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