Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"layeradd layerremove" event handlers dramatically slow down Map data layer rendering #5466

Open
deevroman opened this issue Jan 4, 2025 · 6 comments · May be fixed by #5474
Open

"layeradd layerremove" event handlers dramatically slow down Map data layer rendering #5466

deevroman opened this issue Jan 4, 2025 · 6 comments · May be fixed by #5474

Comments

@deevroman
Copy link

URL

No response

How to reproduce the issue?

I was trying to figure out why the Map Data layer is slow when displaying a lot of elements. In Firefox Profiler, I saw that a lot of time was spent on handlers for adding new objects to the map.

Снимок экрана 2025-01-04 в 21 18 56

https://share.firefox.dev/4a3ZXM9

Specifically:

map.on("moveend layeradd layerremove", function () {
updateLinks(
map.getCenter().wrap(),
map.getZoom(),
map.getLayersCode(),
map._object);
Cookies.set("_osm_location", OSM.locationCookie(map), { secure: true, expires: expiry, path: "/", samesite: "lax" });
});

I don’t understand why these handlers need to update data about the current coordinates, and to prove their uselessness I did the following:

  1. Updated the page with Map Data enabled
  2. Waited for the data to download
  3. Allowed display of a large amount of data

Rendering took a few seconds.

After that, I did the same thing, but deleted the handlers with map.off("layeradd layerremove"). After that, the rendering is understated for less than a second.

osm_perf.mp4

Further movement of the map is also noticeably faster and allows you to display tens of thousands of objects.

Note: map variable is not available by default, it is provided by my better-osm-org extension. This extension doesn't do anything else, and rendering performance doesn't depend on how it works.

You can also analyze the performance profile collected in the video: https://share.firefox.dev/4a3ZXM9

Screenshot(s) or anything else?

No response

@deevroman deevroman changed the title "layeradd" event handlers dramatically slow down Map data layer rendering "layeradd layerremove" event handlers dramatically slow down Map data layer rendering Jan 4, 2025
@AntonKhorev
Copy link
Collaborator

AntonKhorev commented Jan 5, 2025

and to prove their uselessness

You can see that the _osm_location cookie is updated. This cookie, despite its name, also stores selected layers. If you remove some event listeners, the cookie won't be updated on these events.

If you do map.off("layeradd layerremove") then after changing layers, going to some non-map-view page and going back to the map you'll see that your choice of layers wasn't remembered.

The problem is that those layer events happen probably more often than originally anticipated when the data layer is enabled.

@deevroman
Copy link
Author

deevroman commented Jan 5, 2025

What if we add a layer type check to the handler for which the handler was triggered? Something like:

outdated
map.on("moveend layeradd layerremove", function (e) { 
   if (e.layer && !L.TileLayer.prototype.isPrototypeOf(e.layer)){
	  return
   }
   updateLinks( 
     map.getCenter().wrap(), 
     map.getZoom(), 
     map.getLayersCode(), 
     map._object); 
  
   Cookies.set("_osm_location", OSM.locationCookie(map), { secure: true, expires: expiry, path: "/", samesite: "lax" }); 
 }); 

In practice, this does not give an increase, since this is not the only handler :\

@AntonKhorev
Copy link
Collaborator

Maybe listening to "moveend baselayerchange overlaylayerchange" is going to help.

@deevroman
Copy link
Author

deevroman commented Jan 5, 2025

This looks better, but will need to be fixed too.

map.on("moveend layeradd layerremove", update);

And also understand how to track changes in the active object, which should be updated in the link of the OSM editor open button, which the updateLinks function does

window.updateLinks = function (loc, zoom, layers, object) {
$(".geolink").each(function (index, link) {
var href = link.href.split(/[?#]/)[0],
args = Qs.parse(link.search.substring(1)),
editlink = $(link).hasClass("editlink");


Isn't there a way to pause events while the Map data layer is being rendered? https://github.com/openstreetmap/leaflet-osm/blob/ebfaf22426c265aaf3f401d70ffd02ca920623c9/leaflet-osm.js#L107

@mmd-osm
Copy link
Contributor

mmd-osm commented Jan 5, 2025

By the way, since you're looking into map data layer performance, we're also spending some time in XML parsing (DOMParser.parseFromString). Maybe we could switch to /map.json and directly parse the response as json(). Both Rapid and iD are using json for performance reasons.

@deevroman
Copy link
Author

parse the response as json()

This is not necessary yet. I have increased the limit of the maximum number of nodes for the local openstreetmap-cpimap, and I can say that parsing XML, which is 50 MB in decompressed form, only took 2 seconds. However, it took 10 seconds to download.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants