Avoid map repaint when setting same parameter value that is already being used · Pull Request #97 · maplibre/maplibre-gl-js

ghost · 2021-03-15T14:59:03Z

Our map has performance issues due to paint-, layout-properties or filters being touched within a "mousemove"- or requestAnimationFrame handler. Even though we set the same value every time, mapbox will still decide to repaint.
We have many sources and layers, so repainting is slow (see #96) and should be avoided if possible.

While we will likely fix this in our own app by how we use the API, I feel this should also be guarded against by the API itself.
Even mapbox samples themselves do redundant changes in "mousemove" handlers, so I believe that many users will be affected by this.

Internally, mapbox also guards against this (which prevents some computations, but still triggers a repaint).
Mapbox also already guards against redundant changes for setters in the API.

This PR extends the internal checks (more of them) and returns a boolean whether an early-out was taken.
This information is then used to skip map._update which will then avoid an expensive map.triggerRepaint.

I tried to fix as many affected functions that I'm aware of, but I might not have guarded everything. There are also plenty of situations where these guards will fail.

I'm aware of the following shortcomings and potential issues:

setFeatureState will report slightly better errors for unknown keys, because error-checking might already occur in getFeatureState.
The guard for removeFeatureState is very incomplete. Removing a missing-key will trigger repaints anyway. Some code is already present, I just didn't deep dive into the feature-state deletion logic.
Camera transforms using jumpTo are handled, but easeTo isn't.
Changing values back and forth will trigger a repaint, even if it didn't change effectively.
setFeatureState might be considerably slower / unusable if large objects are passed into it. We basically trust that people don't throw window or similar into a feature-state (which would potentially take a long time to compare).
Some users might depend on the old behaviour to trigger map repaints or events.
A related issue is not addressed: Setting unused parameters will still trigger a repaint (= touching an unused source or an invisible layer will still trigger a repaint).
I added a missing return that might affect behaviour for a niche case; I'll leave a review comment to point it out.

Following is code I've been using for testing this feature. I don't check all features though, so there might be bugs left.

Note that master will repeatedly draw with the following code, thereby causing high CPU and GPU usage.
With my proposed change, maplibbre will not draw because changes are redundant.

I have not used this code in production yet, so this has gotten very little testing.
The automated tests in mapbox aren't designed to test for this either (I don't think adding a test for this is necessary though).

<!DOCTYPE html>
<html>
<head>
    <title>Mapbox GL JS debug page</title>
    <meta charset='utf-8'>
    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
    <link rel='stylesheet' href='http://localhost:9966/dist/maplibre-gl.css' />
    <style>
        body { margin: 0; padding: 0; }
        html, body, #map { height: 100%; }
    </style>
</head>

<body>
<div id='map'></div>

<script src='http://localhost:9966/dist/maplibre-gl-dev.js'></script>
<script src='http://localhost:9966/debug/access_token_generated.js'></script>
<script>

var map = window.map = new maplibregl.Map({
    container: 'map',
    zoom: 0.5,
    center: [0, 0],
    style: 'mapbox://styles/mapbox/streets-v10',
    hash: true
});

// Create a GeoJSON source with an simple lineString.
var geojson = {
    'type': 'FeatureCollection',
    'features': [
        {
            'type': 'Feature',
            'geometry': {
                'type': 'LineString',
                'coordinates': [[0, 0], [90, 0]]
            }
        }
    ]
};

map.on('load', function () {
    map.addSource('line', {
        'type': 'geojson',
        'data': geojson
    });

    // add the line which will be "modified" every frame
    map.addLayer({
        'id': 'line-animation',
        'type': 'line',
        'source': 'line',
        'layout': {
            'line-cap': 'round',
            'line-join': 'round'
        },
        'paint': {
            'line-color': '#ed6498',
            'line-width': 5,
            'line-opacity': 0.8
        }
    });

    // Hook so we can log the map repaint behaviour
    const _map_triggerRepaint = map.triggerRepaint;
    map.triggerRepaint = () => {
      var err = new Error();
      console.log(`Repaint at ${performance.now()}; stack was ${err.stack}`);
      _map_triggerRepaint.call(map);
    };

    setRedundantStates();

    // Keep setting the same flags (we expect that this won't trigger a repaint)
    function setRedundantStates(timestamp) {
       
        // Guarded
        map.setRenderWorldCopies(true);
        map.setMaxBounds(undefined);
        map.setMinPitch(0.0);
        map.setMaxPitch(60.0);
        map.setMinZoom(0);
        map.setMaxZoom(100);
        map.setLayerZoomRange('line-animation', 0, 22);
        map.setPaintProperty('line-animation', 'line-width', 5);
        map.setLayoutProperty('line-animation', 'line-cap', 'round');       
        map.setFilter('line-animation', undefined);      
        map.setLight({"color": "white"});
        map.setFeatureState({ source: 'line', id: 'id' }, { 'existing-key': undefined });

        // Guarded (jump transforms)
        if (true) {
            map.setZoom(5);
            map.setBearing(90);
            map.setCenter([-74, 38]);
        }

        // Not guarded yet (ease transforms)
        if (false) {
            map.zoomTo(5);
            map.rotateTo(90);
            map.panTo([-74, 38]);
        }

        // Not guarded yet (broken guards)
        if (false) {
            // Guard exists but it's very incomplete
            map.removeFeatureState({ source: 'line', id: 'id' }, 'missing-key');
        }

        // Not guarded yet (back and forth before map update)
        if (false) {
            map.setMaxPitch(50.0);
            map.setMaxPitch(60.0);
        }


        // Request the next frame of the animation.
        animation = requestAnimationFrame(setRedundantStates);
    }
});

</script>
</body>
</html>

ghost · 2021-03-15T15:01:20Z

        }
        if (target.id === undefined) {
            this.fire(new ErrorEvent(new Error(`The feature id parameter must be provided.`)));
+            return false;


There wasn't any return here originally. This might have API impact if there's a feature with id undefined?

(During development I also came across other spots which used throw instead of this.fire; however, I assume that firing events is usually better)

github-actions · 2021-03-15T15:03:42Z

Bundle size report:

Size Change: +165 B
Total Size Before: 201 kB
Total Size After: 201 kB

Output file	Before	After	Change
maplibre-gl.js	196 kB	196 kB	+165 B
maplibre-gl.css	4.62 kB	4.62 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
src/ui/camera.js	3.15 kB	3.2 kB	+46 B
src/style/style.js	7.09 kB	7.13 kB	+44 B
src/ui/map.js	6.51 kB	6.55 kB	+39 B
src/geo/transform.js	3.7 kB	3.73 kB	+30 B
src/source/source_state.js	625 B	629 B	+4 B
src/source/source_cache.js	4.04 kB	4.04 kB	+1 B

ghost · 2021-03-19T15:08:22Z

This probably needs some additional documentation (whenever we have maplibre docs.. #19).

We now ran into a similar issue for #106 where we occasionally reupload the same ~20MiB GeoJSON. A deepCompare on that is probably a bad idea, but repainting is probably equally bad (I've seen people talking about GiB sized GeoJSON).
So it makes sense to not do these redundancy checks on GeoJsonSource.setData, but it also leads to a mismatch in API behaviour (which must be documented).

I'll create an issue about this after merge, so we can document it in the future.

lseelenbinder · 2021-03-23T07:58:50Z

Hi @JannikGM.

This looks good to me, but I don't consider myself qualified enough in the codebase to be the only reviewer given the number of changes.

ghost · 2021-03-23T08:29:49Z

        if (before && newIndex === -1) {
            this.fire(new ErrorEvent(new Error(`Layer with id "${before}" does not exist on this map.`)));
-            return;
+            return false;


We already did this._order.splice(index, 1); so this should probably return true.

Otherwise, if we move a layer with a bad target it will be removed from the model but the map won't update correctly.

wipfli · 2021-06-05T13:34:47Z

Hi Jannik. The CI should work now. Can you fetch upstream and merge into your branch? Thanks.

HarelM

Looks good to me. If we find other API calls that needs checking we can always add them in the future.

HarelM · 2021-07-05T13:40:51Z

@JannikGM can you merge from main and fix the lint warnings about the jsDocs?
After that I'll merge I think.

HarelM · 2021-09-04T05:36:30Z

@JannikGM can you redo these changes on top of main so I can merge this?
Thanks!

github-actions · 2021-11-04T01:47:16Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2021-12-05T01:50:27Z

This PR was closed because it has been stalled for 30 days with no activity.

Avoid map repaint if redundant state change occurs

c9469cc

ghost commented Mar 15, 2021

View reviewed changes

Avoid map repaint if camera didn't change during jumpTo

b570d25

ghost mentioned this pull request Mar 17, 2021

Map repaint is slow / CPU and GPU intensive #96

Closed

ghost mentioned this pull request Mar 19, 2021

Global flag to disable parameter validation #112

Closed

ghost commented Mar 23, 2021

View reviewed changes

wipfli mentioned this pull request Jun 5, 2021

Support custom tiles source #30

Merged

HarelM closed this Jun 30, 2021

HarelM reopened this Jun 30, 2021

HarelM approved these changes Jun 30, 2021

View reviewed changes

github-actions Bot added the stale label Nov 4, 2021

github-actions Bot closed this Dec 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid map repaint when setting same parameter value that is already being used#97

Avoid map repaint when setting same parameter value that is already being used#97
ghost wants to merge 2 commits intomainfrom
unknown repository

ghost commented Mar 15, 2021 •

edited by ghost

Loading

Uh oh!

ghost Mar 15, 2021

Uh oh!

github-actions Bot commented Mar 15, 2021 •

edited

Loading

Uh oh!

ghost commented Mar 19, 2021 •

edited by ghost

Loading

Uh oh!

lseelenbinder commented Mar 23, 2021

Uh oh!

ghost Mar 23, 2021

Uh oh!

wipfli commented Jun 5, 2021

Uh oh!

HarelM left a comment

Uh oh!

HarelM commented Jul 5, 2021

Uh oh!

HarelM commented Sep 4, 2021

Uh oh!

github-actions Bot commented Nov 4, 2021

Uh oh!

github-actions Bot commented Dec 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ghost commented Mar 15, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 19, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lseelenbinder commented Mar 23, 2021

Uh oh!

ghost Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

wipfli commented Jun 5, 2021

Uh oh!

HarelM left a comment

Choose a reason for hiding this comment

Uh oh!

HarelM commented Jul 5, 2021

Uh oh!

HarelM commented Sep 4, 2021

Uh oh!

github-actions Bot commented Nov 4, 2021

Uh oh!

github-actions Bot commented Dec 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Mar 15, 2021 •

edited by ghost

Loading

github-actions Bot commented Mar 15, 2021 •

edited

Loading

ghost commented Mar 19, 2021 •

edited by ghost

Loading