Mapbox-gl-js: Automatically batch the work done by GeoJSON#setData()

[lucaswoj]

If is relatively common for users to animate geometries on the map using GeoJSONSource#setData

It can be hard to do so performantly and without causing the queue of data update operations to become seriously clogged.

We could make this use of GL JS easier via a newMap#requestSetDataFrame(sourceId: string, callback: Function) method (analogous to requestAnimationFrame).

    function requestSetDataFrame(map, sourceId, callback) {

      var countdown = 2;

      function dataCallback() {
          if (map.style.sourceCaches[sourceId].loaded()) {
              if (--countdown === 0) callback(performance.now());
          } else {
              map.once('data', dataCallback);
          }
      }

      dataCallback();

      requestAnimationFrame(function(now) {
          if (--countdown === 0) callback(now);
      });

   }

[lucaswoj]

cc @kronick

[kronick]

This sounds great and addresses the issue I ran into pretty quickly once I started trying out some relatively complicated animations.

Just thinking about the API for this, ideally I would be able to do:

function animate() {
    // update myData
    source.setData(myData, function() {
        // "Data is updated" callback
        requestAnimationFrame(animate);
    });
}

Would it be possible to integrate the requestDataUpdateFrame() code like this? Otherwise, given @lucaswoj's code above, I picture:

    function animate() {
         // update myData

         map.requestDataUpdateFrame("sourcename", function() {
             requestAnimationFrame(animate);
         });

         source.setData(myData);
    }

Which is awkward 1) because for correctness you should call requestDataUpdateFrame() _before_ setData() (to avoid a race condition that I think is possible if you call it the other way around) and 2) I can't think of a use case where you would want to call requestDataUpdateFrame() without also calling setData()

[lucaswoj]

Would it be possible to integrate the requestDataUpdateFrame() code like this? Otherwise, given @lucaswoj's code above, I picture:

This would be possible syntactically if GeoJSONSource#setData accepted a callback. That is a separate discussion (one which would be valuable to have!)

Conceptually, this nesting would not result in the optimal animation loop. In the worst case, the time between calls to GeoJSONSource#setData would be frame time + data update time whereas we want the worst case to be max(frame time, data update time).

1) because for correctness you should call requestDataUpdateFrame() before setData() (to avoid a race condition that I think is possible if you call it the other way around)

We should debug this race condition. GeoJSONSource#setData is an asynchronous operation so it shouldn't matter whether you call it before or after calling Map#requestDataUpdateFrame.

2) I can't think of a use case where you would want to call requestDataUpdateFrame() without also calling setData()

Agreed 👍

[lucaswoj]

All you should need to do with this API is the following:

    function animate() {
        source.setData(myData);
        map.requestDataUpdateFrame("sourcename", animate);
    }

(note that we call requestAnimationFrame within requestDataUpdateFrame)

[lucaswoj]

In conjunction with this method it may be wise to add a warning when the map's event queue becomes backlogged, suggesting the use of requestDataUpdateFrame.

[ryanbaumann]

@lucaswoj

In conjunction with this method it may be wise to add a warning when the map's event queue becomes backlogged, suggesting the use of requestDataUpdateFrame

Agreed on the requestDataUpdateFrame syntax. For large managing a backlog of data mutation requests, a debounce function could be useful:

_.debounce(requestSetDataFrame(), delayTime, function reportError(e) { console.log(e) } )

@kronick

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

While performance is best with setData() for animations, vector tile filters or style mutations are often appropriate.

[kronick]

@ryanbaumann @lucaswoj

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

Good question. This would make sense to me as a callback to each of those functions. Otherwise we'll have requestDataUpdateFrame(), requestFilterUpdateFrame(), requestPaintPropertyUpdateFrame(), etc. Besides it being a change to longstanding functions in the API, is there an argument against setData(), setFilter(), and setPaintProperty() accepting optional completion callbacks?

[lucaswoj]

For large managing a backlog of data mutation requests, a debounce function could be useful.

I'm not sure I understand. requestSetDataFrame _is_ a debounce function. What's motivating your use of an additional debounce function?

---

Is it possibly to abstract this function to work well with setData(), or setFilter(), or setPaintProperty()?

Good question. This would make sense to me as a callback to each of those functions. Otherwise we'll have requestDataUpdateFrame(), requestFilterUpdateFrame(), requestPaintPropertyUpdateFrame(), etc.

The proposed implementation of requestSetDataFrame works generically with source mutation operations. It would work with setData, setFilter, and setPaintProperty. (With the caveat that setPaintProperty is synchronous and doesn't require this debouncing unless used with a data-driven function).

Can you think of a name for this method that makes it more apparent it can be used with all setData, or setFilter, and setPaintProperty?

---

Besides it being a change to longstanding functions in the API, is there an argument against setData(), setFilter(), and setPaintProperty() accepting optional completion callbacks?

I'm not aware of any strong arguments against completion callbacks.

Completion callbacks are a different feature and deserve a separate discussion in a separate issue. They do not address the same need as requestSetDataFrame. Mutating a source within a completion callback would not come with some of the assurances that requestSetDataFrame does:

• the source's event queue doesn't become backlogged
• the source is not updated faster than the map is being drawn

[ansis]

I think this can be done without adding something to the external api:

From a user's perspective:

function update() {
    map.setData(data);
    requestAnimationFrame(update)
})
update();

implementation:

GeoJSONSource.prototype._queuedData = null;
GeoJSONSource.prototype._currentlySettingData = false;

GeoJSONSource.prototype.setData = function(data) {
    if (!this._currentlySettingData) {
        this._currentlySettingData = true;
        this.actualSetData(data, done.bind(this));
    } else {
        this._queuedData = data;
    }

    function done() {
        this._currentlySettingData = false;
        var data = this._queuedData;
        if (data) {
            delete this._queuedData;
            this.setData(data);
        }
    }
};  

This has the advantage of working automatically without any extra effort from the user. They just call setData like they normally do. The external interface doesn't need to change. Another small advantage is that the queued data will be slightly less stale with this approach, since it'll be the most recent instead of the first.

The downside is that the work the user does before calling setData isn't throttled, but I think this doesn't matter too much. It's usually cheap. And when it's more expensive than setData you wouldn't need to throttle.

requestAnimationFrame could be the browser's api, or we could add a map.requestAnimationFrame that would be guaranteed to get called right before the map gets rendered.

[kronick]

@ansis I really like this option-- especially how it automatically uses data from the most recent call. This behavior is what I would almost always want, and I think you're right that asking the user to throttle their own calls is not a major burden (which requestAnimationFrame() helps with here). It keeps setData() asynchronous (as I understand it needs to be for performance), but without the user having to think too much about what that means under the hood.

[lucaswoj]

Good points @ansis. I'm in favor of automatic throttling as you propose.

The downside is that the work the user does before calling setData isn't throttled, but I think this doesn't matter too much. It's usually cheap. And when it's more expensive than setData you wouldn't need to throttle.

Can you think of any cases where the data update wouldn't be cheap and access to this primitive would be necessary?

[ansis]

Can you think of any cases where the data update wouldn't be cheap and access to this primitive would be necessary?

I can't think of anything right now. And if we add a callback to setData the user could always implement it themselves.

[ChrisLoer]

@lucaswoj @kronick I stumbled into this problem without realizing this issue already existed, and implemented a PR that does setData coalescing on the worker (#5902). While the implementation is a little different, I think the spirit is basically the same as @ansis' suggestion in https://github.com/mapbox/mapbox-gl-js/issues/3692#issuecomment-267403692.

After reading through this issue, I think it'll address the main concerns here without requiring any API changes, but please correct me if I'm missing something.

[ChrisLoer]

5902 has merged and removes the potential for developing a setData backlog, without any API changes.

There are definitely cases where you'd want to do further throttling/debouncing, but it depends on the application. For doing interactive animation with a small set of GeoJSON, you probably don't want any throttling because you want the "frame rate" of the animation to be as high as possible, and the cost of continually reloading the GeoJSON is not that bad. On the other hand, I can image an application that needs to frequently update a large set of GeoJSON but isn't trying to make a smooth animation -- in that case, it might work best for the application itself to implement something like a 1s update frequency.

Please re-open if you think there's still an important case for something like the requestDataUpdateFrame API -- I haven't really thought through the (lack of) callback implications.

[Scarysize]

@ChrisLoer Does this mean we can call setData as fast as possible in the rAF callback without worrying about clogging the workers, which causes stuttering?

Our current workaround was to only request an animation frame, when the source which has been updated (via setData) fired a data event with event.dataType === 'source'. This made it kind of ugly to animate multiple sources.

[ChrisLoer]

@Scarysize This removes one cause of stuttering, but if you're calling setData faster than the background can process it, you'll still have one background worker pegged to ~100% utilization... which may not be ideal.

The difference is that (1) no backlog will develop, and (2) if other (non GeoJSON) tile requests come in to the worker that's handling GeoJSON, they'll get a chance to be processed before the next loadData, instead of having to go to the back of the line.

[Scarysize]

Okay, that sounds like a step in the right direction concerning animations 👍 If you need feedback on further experiments feel free to reach out, we got some projects depending on animations with mapbox-gl :)

[ChrisLoer]

We reverted this fix in #5971 due to regression #5970.

We're working on a fix for the fix in #5983, but it doesn't look like that will make v.44.

/cc @vicapow

[ChrisLoer]

Re-fixed with #5988.

[afanasy]

I'm currently using this to avoid setData congestion (should work for multiple sources too)

function animate () {
  if (!map.isSourceLoaded('source'))
    return requestAnimationFrame(animate) //wait until the next frame
  map.getSource('source').setData(data)
  requestAnimationFrame(animate)
})
animate()

[gorshkov-leonid]

Now we using 0.47.0.
This WA does not help in case of an update on mouse move. For example, you can copy this example in Firefox:
https://www.mapbox.com/mapbox-gl-js/example/mapbox-gl-draw/
Note, It is important, open this example in full screen
I realize that this example uses 'mapbox-gl-draw' plugin. But we have same problems in own implementation with and without of event filtration.