stamen / chartographer

https://stamen.github.io/chartographer/
MIT License
19 stars 4 forks source link

Make layer expansion more performant and/or add a maximum recursion level #37

Closed ebrelsford closed 2 years ago

ebrelsford commented 2 years ago

styles/expandLayers#expandLayers is vulnerable to complex, nested conditional expressions:

https://github.com/stamen/chartographer/blob/e81bb7491f03c4d1caaa72911e5d3ca2adc2ffdb/src/styles/expandLayers.js#L288

This came up in openstreetmap-americana and is likely an issue for styles that use similarly complex expressions. When loading osm-americana each chart hangs, but if you comment out the expandLayers call they display as expected (without accounting for conditions, of course).

There are two ways I'd like to handle this:

  1. Look into making expandLayers more performant. Are there some obvious performance wins we could implement?
  2. Consider adding a floor/maximum recursion level on a layer depending on how (1) goes. At some point we would rather display the charts than have them hang, let's think about a maximum level we would want to go down to that would balance performance/experience.

Separate to the above, it looks like we're expanding layers separately for each chart. If performance is questionable we should probably do this in a common place instead, right?

ebrelsford commented 2 years ago

@aparlato I believe you touched expandLayers, do you have any thoughts about ^?

ebrelsford commented 2 years ago

I added this style to example-styles to make it easier to test with.

aparlato commented 2 years ago

Look into making expandLayers more performant. Are there some obvious performance wins we could implement?

Hmm, I think this is specifically getting stuck on the way we handle property and value combinations rather than nesting alone. It looks like we're just trying to create sometimes hundreds of layers from 1 because of all the available combinations to us in these big expressions 😨

If this really is the biggest thing as I suspect from some testing, then this part of the code would be a good spot to place limits (on the amount of properties/values we try to combine for display).

In addition, if we stick to using the current method for getting combinations of properties and values (after limiting the inputs somewhere beforehand), another small improvement would be to use fast-cartesian which looks like the same library but... faster. Although, this would be negligible compared to the other changes we'll have to make.

I'll take a deeper look at here to confirm and write back with some more details on strategies we could take.

ebrelsford commented 2 years ago

That makes sense to me, thanks for digging into it. I agree that creating the combinations is probably not the slow thing as much as processing the combinations is, but I'd be open to trying fast-cartesian of course.

I'd like to: