Reduce worker payload #7124
Reduce worker payload #7124
Conversation
Leads to further 10% improvement of worker payload.
| @@ -386,7 +394,7 @@ export default class ProgramConfiguration { | |||
| const useIntegerZoom = value.property.useIntegerZoom; | |||
|
|
|||
| if (value.value.kind === 'constant') { | |||
| self.binders[property] = new ConstantBinder(value.value, name, type); | |||
| self.binders[property] = new ConstantBinder(value.value.value, name, type); | |||
There was a problem hiding this comment.
This one's weird — I guess this was a buggy code in an unused code path, since this doesn't break any tests.
There was a problem hiding this comment.
And flow isn't helping us here because ConstantBinder is deducing the type from that first argument? 😕
There was a problem hiding this comment.
Yeah, I guess it's because of flow templating (ConstantBinder<T>) — I'm not fully sure how it works (@jfirebaugh may know better) but looks like if there's a template or an any in Flow, you can basically do anything in the code the touches those places without any type safety.
There was a problem hiding this comment.
Good catch! The problem is we're ignoring the template type in places that would have caught this, e.g. binders is typed as {[string]: Binder<any>} and UniformBindings is typed as {[string]: Uniform<any>}. We'd need more sophisticated per-layer aggregate types like in native to catch this.
This bug manifests when changing a constant property to a data-driven property, in the period before the new layout is completed. We have an integration test for this case (runtime-styling/paint-property-literal-to-property-expression), and I can see the wrong type getting passed into gl.uniform1f, but I guess headless-gl doesn't do strict enough type checking to catch this.
|
|
| const ab = grid.toArrayBuffer(); | ||
| type SerializedGrid = { buffer: ArrayBuffer }; | ||
|
|
||
| Grid.serialize = function serializeGrid(grid: Grid, transferables?: Array<Transferable>): SerializedGrid { |
There was a problem hiding this comment.
I'm missing something... what was the JSON savings in changing from serializing as an ArrayBuffer to serializing as a SerializedGrid?
There was a problem hiding this comment.
If you serialize just the ArrayBuffer which is transferred, any custom properties on it (in this case $name) don't survive the transfer. Before the PR we implicitly transferred in a wrapper ({name: 'Grid', properties: buffer}), and now we explicitly transfer as {$name: 'Grid', buffer}. I'm now throwing an error is serialize in case the serialized object is fully transferred just so that we don't trip up on this in future.
Reduces the size of the JSON (excluding transferred arrays) that gets sent from the worker to the main thread, reducing lags during tile loading (in theory). Partially addresses #7011.
For a dense z11 Mapbox Streets v10 tile in DC, the payload is reduced by ~44%. For the expressions-based Basic style, the reduction is 39%.
See individual commits. The first one has the biggest impact (~28%) — it converts serialized form from
{name: 'Object', properties: {foo: 'bar'}}to{foo: 'bar}for simple objects and{$name: 'Foo', foo: 'bar'}for both registered classes and custom-serialized ones.Launch Checklist
write tests for all new functionalitydocument any changes to public APIs