Performance optimizations

[joux3]

I've been working on some performance optimizations. This pull request contains two of them:

1. reuse styles from the previous invocation if the properties do not differ in the properties that can affect feature styling (the properties that affect styling are calculated dynamically from the style)
2. use the layer of a feature to only test for the styles that use that layer as source-layer

The effect of these two can be compared with the before and after maps. Here's a sample of the JS profiles between these maps:

Are these changes something that could be upstreamed? In some cases at least the style reusing can actually worsen the runtime (namely styles that never match subsequently, but for vector maps with lots of e.g. road features this does not seem to be the case) which might warrant adding it behind a flag. The set of properties that can affect styling also does not handle property functions.

For my use case these optimizations have already made mapbox-to-ol-style "fast enough" for me, but there's still at least some more things to optimize:

1. filter evaluation based on generated JS functions (similar to what mapbox-gl-js does in its feature-filter)
2. figure out why the JIT in Chrome V8 stops using an optimized version of the style function after a while. this would probably require splitting the styling to separate functions

[ahocevar]

I tried your branch with many different styles, but I could not see any noticeable performance improvement. Can you share your style and data please? I'd be interested to see what combination of style/data could benefit from your improvements.

[joux3]

I used the data source and styles as linked in the example maps of the PR description (i.e. MVT tiles by Mapbox as data, OSM bright JS as style). The style used is https://joux3.github.io/mapbox-to-ol-style/perftest/style.js.

Links to the maps repeated for clarity: Before https://joux3.github.io/mapbox-to-ol-style/perftest/before.html After https://joux3.github.io/mapbox-to-ol-style/perftest/after.html

The maps also have a test case (the button on the bottom) that zooms into Moscow that can be used for profiling. On my tests the time taken by the optimized styling function is roughly a third of the original time on latest Chrome.

[ahocevar]

Thanks for the clarification! Based on it, I did some more profiling through all zoom levels of a typical map, and came to the conclusion that only looping through layers whose source-layer matches brings a significant performance gain. I implemented that in a slightly different way with ee19ab24307cc206c7a8d9865614303c448ce8cd. Reusing styles, like you said, does not always improve performance, so I'd be hesitant to do it. Also, the profiler does not list significant time spent in applying properties to existing styles. Instead, I decided to use Mapbox's featurefilter utility. It does not bring much performance gain, but it allows us to no longer pollute `ol.render.Feature#propertieswith a'$type'` property. See 42635c5716d42e19ec8535a516d5c82ef1cbe4c3.

Do you agree with my changes as an alternative to your changes suggested here?

[joux3]

First let me start by saying a thank you for your quick work!

Your layer filter implementation (ee19ab24307cc206c7a8d9865614303c448ce8cd) has a subtle bug: the Z index of a style gets set based on the array index in layersBySourceLayer, not by the array index in the original layers array (63207812f7266cf2d1bc0d9df2bbaf967d0bcaae stores the original array index and uses it when setting Z index of styles). This results in features being drawn with incorrect Z index in some cases.

I also did some profiling based on the different approaches with my data (sorry, I wasn't completely honest when you asked about the style and data as I don't have one that I can share publicly but I could tweak my public test cases be closer to my actual data). This is totally unscientific and just gives a rough estimate on the performance gains:

	Chrome on OS X	IE11
Style reuse + layer optimization	115ms	280ms
Layer optimization + compiled filters	165ms	748ms
Style reuse + layer optimization + compiled filters	88ms	260ms

Based on these results I would certainly like to have style reuse upstreamed. (Though I do understand if IE11 is not used as an optimization target. I unfortunately have to support it). Is there any chance of adding a parameter that would enable the optimization? I could also try to see if I could optimize the cases where style reuse causes performance degradation.

[ahocevar]

First of all, thanks for your continued effort.

Your layer filter implementation (ee19ab2) has a subtle bug: the Z index of a style gets set based on the array index in layersBySourceLayer, not by the array index in the original layers array (6320781 stores the original array index and uses it when setting Z index of styles). This results in features being drawn with incorrect Z index in some cases.

You're right. Fixed with 5a9bea4ce0979aa9fe4da789d79b558642e592fe.

Is there any chance of adding a parameter that would enable the optimization? I could also try to see if I could optimize the cases where style reuse causes performance degradation.

The styles are already being reused. As you can see in the profile, calling the setters for style properties is very cheap, and the styles array does not change when subsequent style function calls produce the same styles.

I still think that the optimisation you implemented only improves performance in rare corner cases, and comes at the cost of quite a lot of additional code that is not trivial to understand and maintain.

[joux3]

The styles are already being reused. As you can see in the profile, calling the setters for style properties is very cheap, and the styles array does not change when subsequent style function calls produce the same styles.

Well, the style objects are being reused but the work done to calculate all of the properties again can take time (for example colorWithOpacity in some profiles seems to take almost a fifth of all the time spent in mapbox-to-ol-style).

I still think that the optimisation you implemented only improves performance in rare corner cases, and comes at the cost of quite a lot of additional code that is not trivial to understand and maintain.

Yep, this is of course a trade-off and you as the maintainer get to decide the course of action to take. Though significant performance degradations should not be caused by this style reusing in the worst case (I tested this by forcing full style evaluation every time) while some cases can easily be 50% faster even on Chrome.

I feel like this pull request has run its course and can therefore be closed. If anybody else is interested in trying out the style reuse optimization, I'll try to maintain a fork at https://github.com/joux3/mapbox-to-ol-style/tree/reuse-styles. I'll also be sure to open new PRs if I come across more general optimizations. Thanks for the great library!

[ahocevar]

Thanks for the hint about colorWithOpacity. I have just pushed 18f4531a0f3fc7df7a5c1391956d6d68b748f764, which drastically reduces the time spent there.