maplibre / maplibre-native

MapLibre Native - Interactive vector tile maps for iOS, Android and other platforms.
https://maplibre.org
BSD 2-Clause "Simplified" License
1.04k stars 300 forks source link

Add guard blocks and checks to `SymbolInstance` #2744

Closed TimSylvester closed 1 month ago

TimSylvester commented 1 month ago

Inserts 64-bit fixed-value guard blocks between each member in SymbolInstance, and checks to regularly validate that these values have not changed.

A certain customer is experiencing crashes in production which nobody has been able to reproduce. The call stacks of these crashes have repeatedly pointed toward SymbolInstance values being corrupted, and the results of these checks confirm that that is occurring and reduces their measured crash rate noticeably, but has not led to any specific problems being discovered.

The failures don't occur at any specific point along the timeline of things being added to or queried in the index. There are many examples of the checks passing just before registerAtCrossTileIndex and failing after, but other cases where they already fail before that. The lack of a specific point hints that the changes are happening from another thread, but it's not clear how that's possible.

In android-10.3.1-pre every accessor validates the guard blocks. Based on feedback from production, the failures of these checks are mostly around CrossTileSymbolLayerIndex::addBucket and addLayer, so I've backed that off and only implemented explicit checks in fewer places.

The checks can be disabled with a preprocessor setting, which should also eliminate any file size overhead of static strings from recording source locations.

github-actions[bot] commented 1 month ago

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1% +6.98Ki  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2744-compared-to-main.txt

github-actions[bot] commented 1 month ago

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.1%  +143Ki  +0.1% +43.1Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2744-compared-to-main.txt

Compared to d38709084a9865fe0bb8300aec70ebf8243b3d43 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +27% +31.1Mi  +420% +25.1Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2744-compared-to-legacy.txt

github-actions[bot] commented 1 month ago

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            +0.0054         +0.0058             0             0             0             0

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-2744-compared-to-main.txt

TimSylvester commented 1 month ago

I was thinking about how to make the guard blocks fail in a test to get the coverage up, but the layout isn't accessible to the API, and I don't see a practical way to do it. @mwilsnd's hooks might be helpful here, if we have a hook where GeometryTileWorker hands over the parsed tile or in placement updating, that passes the bucket through, that would be enough to get at the items and break them.