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

Port Render* split and immutability from native #4875

Open
jfirebaugh opened this issue Jun 23, 2017 · 4 comments
Open

Port Render* split and immutability from native #4875

jfirebaugh opened this issue Jun 23, 2017 · 4 comments
Labels
GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform refactoring 🏗️

Comments

@jfirebaugh
Copy link
Contributor

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

  • immutable, pure style-spec domain objects, independent of rendering, and
  • parallel Render* objects that handle rendering-specific concerns and can update themselves by diffing prior and new immutable objects

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

@anandthakker
Copy link
Contributor

anandthakker commented Apr 17, 2018

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:

  • Treat the Style > StyleLayer > Layout/Transitionable class hierarchy as private implementations (StyleImpl > StyleLayerImple > etc.)
  • Expose each one to rest of the codebase through a pair of interfaces:
interface StyleLayer {
  /* getters, but no setters */
  getMutable(): MutableStyleLayer;
}
interface MutableStyleLayer extends StyleLayer {
  /* setters */
}
class StyleLayerImpl implements MutableStyleLayer {
   /* getters and setters */
   getMutable(): MutableStyleLayer { this.shallowClone(); }
}
  • Where we need similar semantics for raw objects or arrays, use type MutableBlah = ...; type Blah = $ReadOnly<MutableBlah>, together with a asMutable<T>(x: $ReadOnly<T>): T { return shallowClone(x); }

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
Copy link
Contributor

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
Copy link

mb12 commented Apr 18, 2018

@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
Copy link
Contributor

anandthakker commented Apr 19, 2018

@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:

  • Better separation of concerns, with pure style-spec domain objects responsible only for modeling a style, and the Render* hierarchy responsible for tracking rendering-specific state
  • Simplify--or, rather, eliminate--the complicated state/logic that's currently required to properly track and apply runtime styling changes.
  • Relatedly, this paves the way for unifying the more granular, imperative parts of the runtime styling API under the diff-based "smart setStyle"

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform refactoring 🏗️
Projects
None yet
Development

No branches or pull requests

3 participants