Performance regression: feature-state _idMap building is expensive

[go-d]

Since version 0.46, when feature-state was added, time-to-first-render (TTFR) drastically increased.

Not building _idMap in populatePaintArrays in ProgramConfiguration, by getting rid of the if-block there, gets you back the old performance.

Could you make it so _idMap doesn't get built unless feature-state is actually used? Or maybe delay it until after first render?

See also: #7039 (comment)

BTW, #7015 helps reduce TTFR, but slowdown is still significant compared to version 0.45.

[mourner]

#7124 might also somewhat help. @go-d can you describe how exactly you measure TTFR so that we're aligned on measuring impact of various changes?

[go-d]

How I measure:

console.time(map);
map.on('load', console.timeEnd);

For my data, to get some idea of the impact:

• master: ~5.3s
• reduce-worker-payload: ~4.6s
• serialize-feature-id-map: ~1.6s
• master, patched¹: ~1.2s
• master, patched²: ~1.7s

[1]

--- a/src/data/program_configuration.js
+++ b/src/data/program_configuration.js
@@ -385,7 +385,7 @@ export default class ProgramConfiguration {
         for (const property in this.binders) {
             this.binders[property].populatePaintArray(newLength, feature);
         }
-        if (feature.id) {
+        if (false && feature.id) {
             const featureId = String(feature.id);
             this._idMap[featureId] = this._idMap[featureId] || [];
             this._idMap[featureId].push({

[2]

--- a/src/data/bucket/fill_bucket.js
+++ b/src/data/bucket/fill_bucket.js
@@ -72,6 +72,7 @@ class FillBucket implements Bucket {
                 options.featureIndex.insert(feature, geometry, index, sourceLayerIndex, this.index);
             }
         }
+        for (var pc in this.programConfigurations.programConfigurations) this.programConfigurations.programConfigurations[pc]._idMap = {};
     }
 
     update(states: FeatureStates, vtLayer: VectorTileLayer) {

[mourner]

BTW, #7015 helps reduce TTFR, but slowdown is still significant compared to version 0.45.

@go-d thanks! So, #7015 does almost bring it down to pre-featurestate levels? 1.6s vs 1.2–1.7s.

[mourner]

@go-d it also looks like it heavily depends on the style the map uses — I don't get anywhere near results like that with the usual mapbox-streets-v10. Can you share the style id you're using?

[go-d]

@mourner No, patch 1 and patch 2 are independent tests. So #7015 is still quite a bit slower: 1.6s vs 1.2s pre-featurestate. Also not at all dependent on style.

var map = new mapboxgl.Map({
  container: 'map',
  renderWorldCopies: false,
  style: {
    "version": 8,
    "sources": {
      "source1": {
        "type": "vector",
        "tiles": ["http://localhost/{z}/{x}/{y}.pbf"],
        "maxzoom": 0
      }
    },
    "layers": [
      {
        "id": "layer1",
        "type": "fill",
        "source": "source1",
        "source-layer": "a",
        "paint": {
          "fill-opacity": 0.1
        }
      }
    ]
  }
});

Single tile used for testing:
0.pbf.gz

[mourner]

@go-d thanks for a great test case! It makes it clear that TTFR directly depends on the JSON size that's transferred — without feature ids, it transfers 8kb, and with it, 15.5MB (!). I'm now experimenting with a different data structure for id mapping to see if we can get close to former performance...

Also not at all dependent on style.

I meant including the underlying tile data used.

[mourner]

@go-d please check out #7132 — it should be on par or faster than #7015 while also avoiding frame drops.

[go-d]

@mourner

• transferable-feature-map: 1.2s

So same, but rounded off, so did a couple measurements: getting 7% slower with #7132 compared to pre-feature-state. Looking good.

Lower than the 10-20% you got. Tried Chrome instead of Firefox and got 16%.

[mourner]

@go-d great!

Thinking about further improvements — I'm not sure if we'll manage to delay this work until after the first render because it would mean reparsing all the tiles again, which seems more wasteful. Avoiding it completely would also require full reparse on the first setPaintProperty with feature state. So perhaps we have to just live with a 7-16% drop — at least it would be manageable (compared with a 4x slowdown).