mapbox / mapbox-gl-js

Interactive, thoroughly customizable maps in the browser, powered by vector tiles and WebGL
https://docs.mapbox.com/mapbox-gl-js/
Other
10.92k stars 2.2k forks source link

Port Render* split and immutability from native #4875

Open jfirebaugh opened 7 years ago

jfirebaugh commented 7 years ago

We should replace the current smart setStyle implementation in gl-js with one more similar to native, based on a split between:

This approach eliminates the complex state tracking needed to track "pending" changes (Style#_updatedLayers, _updatedSymbolOrder, _updatedSources, etc.).

anandthakker commented 6 years ago

I've been thinking a bit about how we should express immutability semantics for this work. gl-native uses a set of generic classes Mutable<T>, Immutable<T> (for immutable objects), and Collection<ClassWithImmutableImpl> (for a collection capable of perf benefits from immutability-awareness). Together, these provide:

  1. Static enforcement of immutable state management
  2. Zero runtime overhead (relative to pre-existing implementation -- I think)
  3. DRY implementation, abstracted into the general-purpose classes linked above

So far, it looks to me like we can probably only get two out of three of these with JS/Flow (specifically, I don't see a way to get 3 without compromising 2). I'm leaning towards an approach where we:

Updating state would then look like:

setPaintProperty(layerId, name, value) {
  // this.layers[layerId].setPaintProperty() would fails, because this.layers[layerId] is typed as StyleLayer, which has no setters
  const layer = this.layers[layerId].getMutable();
  layer.setPaintProperty(name, value); 
  const layers = asMutable(this.layers);
  layers[name] = layers;
  this.layers = layers;
}

Slightly more detailed example based on our Layout properties class: https://gist.github.com/anandthakker/5d5a18392a099ff25479bad9dca01311

anandthakker commented 6 years ago

One gotcha we'll have to watch out for is that if a type T is typed as Object (class Foo<T: Object> {...}), then flow won't protect against writes to $ReadOnly<T>. (Example h/t @jfirebaugh.) A workaround, present in the example linked above, is to use {[string]: mixed} instead of Object.

mb12 commented 6 years ago

@anandthakker What kind of safety would an Immutable wrapper provide in GL JS? In JS the rendering thread and main thread is the same. The default web worker semantics of web workers are copy. Is the goal to make all data structures accessed by rendering loop wrapped by Immutable to prevent them being accidentally changed by DDS api (Again this should not happen if the main thread and rendering thread is the same). Is it possible to explain what is the design goal at a high level/ what kind of bugs this will prevent?

anandthakker commented 6 years ago

@mb12 sure. It's true that thread safety isn't relevant to GL JS. While that was the original motivation for the equivalent gl-native changes, there are other significant benefits to this architecture.

@jfirebaugh alludes to some of them above:

Immutability in particular supports (a) simpler and more efficient style diffing, and (b) solving problems involving style state synchronization / consistency (https://github.com/mapbox/mapbox-gl-js/issues/4012 https://github.com/mapbox/mapbox-gl-js/issues/6367 https://github.com/mapbox/mapbox-gl-js/issues/6255) very cleanly--namely, by ensuring that any reference to a style or layer is a reliable snapshot.