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

Use baselayerchange/overlaylayerchange instead of layeradd/layerremove #5474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

deevroman
Copy link

@deevroman deevroman commented Jan 6, 2025

Description

This is fix #5466 After merging openstreetmap/leaflet-osm#43 flame graph looks like this:

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

Most of the time is spent on two handlers that update information about the current location and elements in the right sidebar.

Unfortunately, replacing layeradd layerremove with baselayerchange overlaylayerchange (#5466 (comment)) is difficult, since in addition to the Map Data layer, other elements may be displayed on the map. For example, the currently selected object on the map. It is not clear how to separate changes to Map Data objects and changes to the selected element. It is necessary to track changes in the active element, otherwise at least the Edit button will break.

Therefore, I implemented a little ugly, but safer in terms of breaking business logic. When adding the Map Data layer, I set the pause flag in the slowdown handlers. While the Map Data is rendering, the user cannot interact with the interface, so it seems safe to pause the event handlers.

Total 15s vs 0.5s when rendering 5000 elements.

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

@deevroman deevroman marked this pull request as ready for review January 6, 2025 14:00
@AntonKhorev
Copy link
Collaborator

For example, the currently selected object on the map. It is not clear how to separate changes to Map Data objects and changes to the selected element. It is necessary to track changes in the active element, otherwise at least the Edit button will break.

addObject/removeObject in leaflet.map.js could fire some event for active object change. The listener that calls updateLinks could listen to that event.

@deevroman
Copy link
Author

I also saw a problem in PR: when you click the Map Data checkbox, the cookie with the current location is no longer updated

@deevroman deevroman marked this pull request as draft January 6, 2025 19:57
@deevroman deevroman force-pushed the speedup-datalayer-render branch 5 times, most recently from a359de9 to a32249e Compare January 10, 2025 23:27
@deevroman deevroman changed the title Suspend the layer event handlers when rendering Map Data Use baselayerchange/overlaylayerchange instead of layeradd/layerremove Jan 10, 2025
@deevroman deevroman marked this pull request as ready for review January 10, 2025 23:51
@deevroman
Copy link
Author

deevroman commented Jan 10, 2025

Okay, firing events from addObject / removeObject turned out to be the best idea. And I changed this PR to use baselayerchange/overlaylayerchange

I also replaced map.on("layeradd",...), which tracks the layer's on/off state with dataLayer.on("add", ...). Using overlaylayerchange in this case doesn't work, because this event isn't thrown when opening a new page with ...&layers=D Plus add / remove looks cleaner in my opinion.

@deevroman deevroman marked this pull request as draft January 10, 2025 23:59
@deevroman deevroman force-pushed the speedup-datalayer-render branch from a32249e to 5b37b54 Compare January 11, 2025 00:11
@deevroman deevroman marked this pull request as ready for review January 11, 2025 00:15
@mmd-osm
Copy link
Contributor

mmd-osm commented Jan 11, 2025

With the latest changes I was able to render Lake Huron locally in a reasonable amount of time (<20s, including 4.5s API response time; 455k nodes). Also map data display is extremely fast now (e.g. 100ms instead of 3s). That is definitely a significant speed-up.

image

Furthermore, switching from XML to JSON has improved the response time from 20s to 8s (both values include 4.5s API time). For reference, I've uploaded the quick hack here: https://gist.github.com/mmd-osm/d45ff3b4f41908b1c075d70d1b6fdebe (edit: also added fetch based alternative).

image

We should think about adding some error handling:

image
image

@HolgerJeromin
Copy link
Contributor

Jquery uses JSON.parse for parsing of ajax() result.

In a next step switching to `fetch" would allow using
https://developer.mozilla.org/en-US/docs/Web/API/Response/json
Where the parsing is done without blocking the main thread.

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

Successfully merging this pull request may close these issues.

"layeradd layerremove" event handlers dramatically slow down Map data layer rendering
4 participants