Use a special data structure for transferring feature id maps to the main thread #7132
Conversation
|
Can we avoid even generating an id map for layers that don't use any |
|
@jfirebaugh that is possible, but would require re-building the paint arrays, or building the map some other way if |
|
@jfirebaugh we should definitely explore this, but independently of this PR (which will be a benefit in any case I think). |
|
Did none of our existing benchmarks detect the performance regression in #7110? What would it take to have a benchmark that can alert us to regressions in TTFR? |
src/data/program_configuration.js
Outdated
| } | ||
|
|
||
| this._bufferOffset = newLength; |
There was a problem hiding this comment.
The _bufferOffset needs to move forward after each feature is added, even if there is no id attribute. For tiles that have features mixed with/without ids, the current code will assign all vertex array positions between two features with ids.
Here's an example of this on a symbol layer:
| } | ||
| } | ||
| const positions = []; | ||
| while (this.ids[i] === id) { |
There was a problem hiding this comment.
Would be nice to add a note here that it relies on sorting the ids and positions arrays as part of serialization. Maybe include a test to ensure that serialize sorts these.
@jfirebaugh it might help to add a worker roundtrip benchmark that parses a few heavy tiles and then just sends them back and forth between the main thread and a worker (to specifically target |
|
@mourner We should have a benchmark that shows this change is a performance benefit though. (And unit tests.) |
mourner
force-pushed
the
transferable-feature-map
branch
from
6766695
to
784293d
Compare
|
Results for the new WorkerTransfer benchmark:
|
Fixes #7110 (not fully but almost there); a part of addressing #7011. An alternative to #7015.
Instead of a struct array that gets deserialized into a JS object on the main thread (which is expensive and causes stutter), this PR introduces a special data structure for storing feature id to position mappings which consists of two typed arrays — ids and positions. Both get sorted by id before transferring, which allows using binary search on the main thread for id lookup (avoiding building a JS object completely).
This brings TTFR for the difficult #7110 case to nearly pre-feature-state levels (10-20% slower instead of 4x slower), while also theoretically reducing stutter (due to elimination of big non-transferable id maps which are very expensive to clone when sending to the main thread). @ryanbaumann's feature state choropleth also speeds a little (20% faster to fully load).
Note that I used
Uint32Arrayfor buffer offsets instead Uint16 (like in #7015) because the latter overflows (e.g. on Ryan's example).Launch Checklist
document any changes to public APIs