Cache style layers and sources

[LukasPaczos]

The pre-v10 Style object cached layers and sources on the platform side to avoid having to fetch them from native whenever getter methods were called (refs https://github.com/mapbox/mapbox-gl-native/pull/13484).

In v10, this is not available anymore and as https://github.com/mapbox/mapbox-navigation-android/pull/4425#issue-649021787 shows, the constant recreation of style objects (getStyleLayerProperties in this case) causes a considerable performance penalty when called frequently (on each animation frame in this case).

It'd be great if we can reintroduce some sort of platform caching or measure the performance of the native getStyleLayerProperties to see whether just crossing the JNI boundary and marshaling is the bottleneck or whether the native method itself could be optimized.

cc @mapbox/maps-android @cafesilencio

[LukasPaczos]

Adding a more detailed profile screenshot:

[pengdev]

We have these values cached in the style layer and source classes already, just for consistency we decided to go through the gl-native to get the latest property values, since they could be modified in other places. Do you think we should always return the cached values? Or shall we introduce some flags to toggle the cache/without cache getters?

Optionally, I think we could also expose the entire cached Property HashMap if that's helpful.

[LukasPaczos]

Property values are one thing (that I didn't measure) and it does make sense to always ask GL-Native for that since it's the source of truth. The concern here is about obtaining the layer/source reference itself (StyleManagerInterface#getStyleLayerProperties). Whenever a layer reference is first obtained from native, we could cache it in a map on the platform side so that all subsequent calls return a cached value instead of asking GL-Native again. We'd have to only drop that cached reference when removeLayer/Source is called or the style is reloaded.

[LukasPaczos]

We could also use styleLayerExists/styleSourceExists before returning the cached reference since those methods seem to be much faster.

[pengdev]

Hi @LukasPaczos I tried to read through the ticket again, I think the bottleneck is the call of getStyleLayerProperties when you are using style.getLayer(layerId) API. The getStyleLayerProperties is pretty heavy since it tries to retrieve all the layer properties. I think we can optimise it to use two simple and lighter getStyleLayerProperty APIs, since we just need the layer type and source to construct the Layer.

I'm working on a PR to optimise it at https://github.com/mapbox/mapbox-maps-android/pull/449 , do you want to do some benchmarks using a snapshot build?

[LukasPaczos]

@pengdev happy to take it for a spin whenever you think it's ready :+1:

[pengdev]

@LukasPaczos thanks! I have triggered a snapshot, you can try 10.0.0-rc.1-peng-improve-getLayer-api-SNAPSHOT when you have time.

[pengdev]

@LukasPaczos I did some profiling on this change, in the same benchmark, the CPU utilisation seems to be improved

Before(getLayer takes 12.1% CPU time, getStyleLayerProperties takes 8.7% CPU time):

After(getLayer takes 5.47% cpu time, getStyleLayerProperty takes 2.14% CPU time):

[LukasPaczos]

My benchmarks integrating the snapshot into the Nav SDK are even more promising given the frequency with which we call all the methods.

Before

After

A decrease of ~42 percentage points of time spent in the getStyleLayerProperties!

It still is not negligible, and now the bottleneck becomes the setStyleLayerProperties, but definitely the right direction.

[pengdev]

@LukasPaczos What's the code path to the setStyleLayerProperties call? I can try to look where we can optimise.

[tobrun]

@LukasPaczos What's the code path to the setStyleLayerProperties call? I can try to look where we can optimise.

https://github.com/mapbox/mapbox-navigation-android/blob/79ab96bfb3f9a05aa5b6a7ae9e10124a36b4c756/libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/api/VanishingRouteLine.kt#L173-L192

Which relates to code found in https://github.com/mapbox/mapbox-navigation-android/blob/d10b9aeb7c079307d115ed2549e298337a450530/libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/internal/route/line/MapboxRouteLineUtils.kt#L129

[LukasPaczos]

And then the generated expression is set here: https://github.com/mapbox/mapbox-navigation-android/blob/6d041e3f11ef0ae200aa97a57ae02f26b49ff658/libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineView.kt#L328-L334

[pengdev]

Setting the lineGradient and the under hood setStyleLayerProperty seems not avoidable on the platform side. @LukasPaczos could you suggest how I could simulate the navigation session with the gradient line in use? I will try to run the exact the same code and do more benchmarking.

[LukasPaczos]

@pengdev you can try updating the gradient every time the OnIndicatorPositionChangedListener fires. We do that for 3 line layers in the Nav SDK.

[pengdev]

The performance of setStyleLayerProperty with complex expressions seems to be the main bottle neck currently, we need to solve it by optimising the Value marshalling.