when using style function on layer, it always needs to return an array #1838

Closed
bartvde opened this Issue Mar 11, 2014 · 5 comments

Projects

None yet

3 participants

@bartvde
OpenLayers member

So if people just return an ol.style.Style object, things will not work. Is there a way to make it work with both an ol.style.Style object as well as an array of ol.style.Style objects?


Want to back this issue? Place a bounty on it! We accept bounties via Bountysource.

@bartvde bartvde added the api label Mar 11, 2014
@twpayne

There is a way to do this, but it will be bad for performance, causing regular de-optimisations.

@bartvde
OpenLayers member

Okay, is there any way to fallback to the default style? Or to output a warning or something that will point people in the right direction?

For a novice user it's a pretty common case to run into I guess.

@tschaub
OpenLayers member

If we want to support this, it can be done without any extra pressure on the garbage collector. I also don't see how this would lead to frequent deoptimization. If you're talking about optimization related to the styleFunction itself, I imagine the opportunity for maintaining the optimized version would be the same whether it was always returning an array or always returning a single style.

See my style branch for an example on how this could be supported.

@twpayne

The deoptimisation is related to how VMs generate code. Basically, if the types are consistent then they can generate very fast assembly code. However, if the types change (e.g. you get an ol.style.Style where previously you were getting an Array.<ol.style.Style>) the VM has to throw away the fast assembly code and replace it with slower, more general code. See Breaking the JavaScript Speed Limit with V8 for details, specifically the section on polymorphic inline caches.

@tschaub
OpenLayers member

Right @twpayne - that's consistent with my understanding. So deoptimization is only more likely if the style function sometimes returns an array and sometimes returns a style instance. I don't think this is what we're talking about here.

@tschaub tschaub removed the api label Sep 21, 2015
@tschaub tschaub closed this in #4401 Nov 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment