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

WIP: Performance tuning: rendering worldmap objects #253

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

vpithart
Copy link
Contributor

@vpithart vpithart commented May 22, 2020

Why?

It's slow. Typical workflow on the worldmap: zoom in, zoom in, zoom in, drag, drag, drag, zoom out ... had caused excessive browser load when re-rendering all the objects on the map.

How?

  • UI and call_ajax() synchronization. After fast successive map moves, only last of (often overlapping in time) getMapObjects' results are being rendered.
  • Avoid double render. After a map view is dragged (not zoomed), most of the objects stay visible. Only those new are rendered, and old (out of viewport) are removed.
  • Render less out-of screen objects. Extra area shrunk from 95% to 50%.
  • Newer leaflet. The [leafletjs] library is upgraded from ancient v0.7 to v1.6.

@ekrichbaum
Copy link
Contributor

I was thinking about the leaflet cleanup as well. I like pulling it back out to a lib that can be more easily swapped for versions... I'm going to merge this into mine and give it a run. Thanks.

@ekrichbaum
Copy link
Contributor

share/frontend/nagvis-js/js/ViewMap.js 128 this.removeAllObjects()
is missing ;

My maps aren't heavy enough to really see much performance change, I think, but no ill effects for sure.

@ekrichbaum
Copy link
Contributor

well. may have spoken too soon but haven't narrowed down when it happens. do a lot of zoom in and out with movement around and get a second copy of all the lines and objects overlayed.

image

Seems to be related to zoom as I haven't been able to get it from just moving around and they seem to alway be different scales. Doesn't seem related to the ElementLine changes at least but it'll take a while to put these back in and out to narrow further.

@ekrichbaum
Copy link
Contributor

Doesn't seem to have related to the ViewMap changes.

@ekrichbaum
Copy link
Contributor

Seems to be in the ViewWorldmap changes. Another missing ; after this.last_zoom = g_map.getZoom() line 116

Adding back the this.render(); // re-render the whole map
seems to correct the issue.

@ekrichbaum
Copy link
Contributor

sorry for the run ons. and:

        if (this.last_zoom !== g_map.getZoom())
            this.handleZoomEnd(lEvent)
        else
            this.handleDragEnd(lEvent)

needs ;
and {} for style conformity.

@ekrichbaum
Copy link
Contributor

ekrichbaum commented May 24, 2020

Hmm. And don't seem to be able to hover the lines or icons anymore. Reverting these changes completely at this point.

Reverting ViewWorldmap.js got hover back as well. I'm going to leave currently with all of the changes except ViewWorldmap.js. Haven't seen any other ill effects at this point that way.

@vpithart vpithart changed the title Performance tuning: rendering worldmap objects WIP: Performance tuning: rendering worldmap objects May 25, 2020
@vpithart
Copy link
Contributor Author

Thank you @ekrichbaum for testing it too. I' got also double rendering issue, not easily reproducible so far; working on it.

@vpithart
Copy link
Contributor Author

vpithart commented May 25, 2020

I quit an attempt to optimize worldmap rendering. Although partial rendering after moveend map event is faster, I'm out for now. It causes too many usability problems (misplaced objects moved after map drag, misplaced new objects, wrong coordinates, ...). No quick fix seems feasible.

@vpithart vpithart changed the title WIP: Performance tuning: rendering worldmap objects Performance tuning: rendering worldmap objects May 27, 2020
@LarsMichelsen
Copy link
Contributor

Shall we close the pull request? Or are there still some commits that we should take over?

@vpithart
Copy link
Contributor Author

It's still to merge. Of the 4 planned enhancements (UI and call_ajax() synchronization, Avoid double render, Render less out-of screen objects, Newer leaflet) only "avoid double render" is reverted, and the rest is ready.

The performance boost is just slight though.

To really boost it, rendering of the worldmap objects would need complete rework and perhaps tighter integration with leaflet. Maybe later.

@vpithart vpithart changed the title Performance tuning: rendering worldmap objects WIP: Performance tuning: rendering worldmap objects Jun 1, 2020
@vpithart
Copy link
Contributor Author

vpithart commented Jun 1, 2020

We've fond dome minor compatibility problems with the new leaflet. Marking this WIP until thoroughly tested.

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.

3 participants