Evaluate conditions in config without using `eval`
Closed this issue · 13 comments
We have a cool feature that can change the styles of our layers through the config, using a condition property, like this: - condition: '{{map.zoom}} >= 14' (shown here: https://github.com/smartercleanup/duwamish/blob/master/src/flavors/duwamish_flavor/config.yml#L390)
Unfortunately, we are using a javascript eval statement to evaluate the conditional, and it greatly slows down our map. This can be seen every time the map loads, or the zoom changes on the map. Here is the eval statement: https://github.com/smartercleanup/duwamish/blob/master/src/sa_web/static/libs/leaflet.argo.js#L186
Instead of evaluating that string literal, instead we can load a javascript function into that config YAML file (which eventually get processed into a Python dict, then a javascript object). I am still not sure exactly how the solution will work, but any suggestions are welcome!
I don't know if this is on the right track, but it's possible to define and execute a function from a string using the Function() constructor. So you can do stuff like:
var fn = new Function("a", "b", "return a + b")
fn(4, 5) // 9
So in this case, perhaps we could define a function in the config with all the checking logic inside of it for each place type, then feed it {{layer.focused}} and {{map.zoom}} (or whatever else is relevant) as parameters when it executes. I don't know if this would perform faster than eval though. It might be a wash. But I've read that the preference is to avoid eval()--although I'm not too clear on the specific reasons for this.
@goldpbear I wish I saw your comment yesterday, but I did some research, and ended up with using this approach. Thank you for the suggestion.
@Lukeswart You may close this issue if you believe it is resolved.
This is a very interesting idea! Thanks for looking into it. I overlooked goldpbear's comment last night as well. I can investigate it a bit more and see whether there it affects performance. I am a bit skeptical that the javascript runtime will still need to open up a new interpreter to evaluate that string conditional...
@teamomivida @Lukeswart I think you're right that it won't be necessary to open a new interpreter using Function(). I'll be curious to learn what performance implications this has over eval--let me know if you guys end up doing any testing on it!
@goldpbear @Lukeswart
I did a very simple test assigning performance.now() before and after running Function() and subtracting the difference, then did the same with eval(). it seemed like the Function() is one third faster, but the test is not that accurate, so I'd like to learn what's the best way I can test it.
Cool! I'd guess that a one third faster loading time would be noticeable to the end user on maps with a lot of content.
I don't know very much about performance testing, but the docs for Performance.now() make it sound like a pretty good approach. I suppose running multiple tests and averaging the times would produce a more accurate measurement, but I'd be kind of surprised if your initial measurement was very far off the mark.
While the "Function" solution is an interesting alternative, after looking into it, I believe it still shares the same performance issues as eval. Here is some information that I found:
Another problem is the performance hit that eval incurs because it must parse, evaluate, then interpret, source code of unpredictable size - it may contain few statements, or very many. Function shares this problem.
taken from this post: https://dfkaye.github.io/2014/03/14/javascript-eval-and-function-constructor/
which makes sense, because in either case we are still passing in a string that needs to be dynamically evaluated into Javascript code. And I don't see how new Function will magically solve this problem without having to stop execution and open up an interpreter under the hood.
This also might be related:
Function objects created with the Function constructor are parsed when the function is created. This is less efficient than declaring a function with a function expression or function statement and calling it within your code, because such functions are parsed with the rest of the code.
So I think we'll still need to parse the YAML file into Javascript code to avoid evaluating it dynamically, which we started discussing last Thursday night. But I may be missing something, because I'm not sure how to explain the 1/3 increase in performance. Perhaps Function might work after all... but I don't know why. @teamomivida maybe you can share a code snippet, or push up a branch like eval-performance-testing, so that we can replicate your test? Performance testing will definitely be very helpful in solving this problem!
I see-- so the issue is that the interpreter still has no way of knowing what's coming inside Function(), just as with eval. That makes sense.
In the first link above there are some notes about eval's performance compared to Function. It sounds like there's a lot of variability--different browsers/runtimes handle things differently. It notes that Function() is supposedly the slowest option in Firefox, but might be faster in other environments.
So maybe we should run the performance tests in a few browsers to see if there are noticeable differences.
Just as a followup thought to this-- it seems like in general we have a situation where we want to map a continuous input domain (i.e. the map's zoom level) to a discrete output range (the icon sizes and anchor points). So if we want to avoid dynamically evaluating strings in the config that explicitly encode specific conditions, perhaps we can use a scaling function that does the mapping we need.
The D3 dataviz library has a "quantize" function that does this (https://www.npmjs.com/package/d3-scale#quantize-scales). I'm not saying we should use D3 here (although it looks like there's a standalone package for just the scales), but maybe we could write a version of the quantize scale. That way, we would just plug in the various domain and range values into the config as simple values, and read them out of the js object when we run the scaling function.
Ooh, nice idea! But when you say this:
it seems like in general we have a situation where we want to map a continuous input domain (i.e. the map's zoom level) to a discrete output range (the icon sizes and anchor points)
I don't think our input is always continuous, nor is it always numerical. For example, how would we replace conditions like this one: "{{properties.marker-color}}" == "a3e46b" or this one: "{{geometry.type}}" == "Polygon"? In those examples, the inputs (properties.marker-color and geometry.type) are not numbers, but arbitrary strings. And what about this case: "{{properties.marker-color}}" == "a3e46b" && "{{geometry.type}}" == "Polygon"`? In that condition, we'll have two inputs.
The D3 solution would work great for the map zoom configurations, but it would still be limited to only that type of option. For example, I'm not sure how we could change an icon's size based on whether it is focused. Here is an example of what we would have in the config:
georgetownZoomStyle:
- condition: '{{layer.focused}} === true'
icon:
iconSize: [60, 60]
iconAnchor: [30, 30]
- condition: '{{map.zoom}} < 14'
icon:
iconSize: [7.5, 7.5]
iconAnchor: [4, 4]
- condition: '{{map.zoom}} < 16'
icon:
iconSize: [15, 15]
iconAnchor: [8, 8]
- condition: '{{map.zoom}} >= 16'
icon:
iconSize: [30, 30]
iconAnchor: [15, 15]
If we want a user to style markers in the config using arbitrary marker properties, then I think we'll have to develop a parser (or deal with the performance of using "eval"). Unless we can think of any other options? Let me know what you think, or we can also followup on this in person at the next meetup.
@Lukeswart -- Ah, you're totally right. Sorry, I didn't look through all the different conditions thoroughly enough.
Yeah, the variety of conditions definitely limits the usefulness of the scale solution. Technically, for conditions where there's a one-to-one mapping (like the properties.marker-color conditions), I think a scale could still be used. D3 has ordinal scales for discrete-to-discrete mappings, for instance.
However, you'd still need to tell it which type of scale to use in each condition, and you're right that none of that would be useful anyway for more complex conditions (or even for simple ones, like {{layer.focused}} === true, that don't consist of mapping one value to another).
I guess a parser would be the more versatile solution. I can't think of another approach at the moment.
It looks like a parser already exists: https://github.com/substack/static-eval
We can probably import that via bower, and replace our eval function with it
If that doesn't work, here is the start of a function that might be helpful:
// Returns whether the condition is true or false
function evalCondition (condition, logicalOperator, existingCondition) {
// Use a greedy operator `(.+)?` to only match on the first regex split:
var equations = condition.split(/( \|\| | \&\& )(.+)?/)
// Check to see if we are at the last equation:
// (this is the base case for our recursive function)
if (equations.length < 3) {
evalEquation(equations[1], logicalOperator, existingCondition)
}
equations[3].split(/( \|\| | \&\& )(.+)?/)
if (existingCondition !== undefined) {
existingCondition = evalEquation(equations[1], logicalOperator, existingCondition)
var newCondition = equations[0]
var newLogicalOperator = equations[1]
return evalCondition(newCondition, newLogicalOperator, existingCondition)
}
}
var operators = ['<', '>', '===', '==', '!=', '!==']
// TODO: Parse our single equation to return true or false
function evalEquation(equation, logicalOperator, existingCondition) {
}
// Usage example:
var condition = "12 > 14 || 12 < 16 && 'red' === 'blue' "
evalCondition(condition.trim())Since we were talking about map performance the other day, it got me thinking about this issue again.
What if we have a set of global zoom styles that apply to all layers and all icons? We could set global threshold zoom levels which would trigger a new zoom style to be applied, but between the threshold levels no new styling would need to take place. We could maintain certain things like per-layer anchor points and icon sizes, but this approach would eliminate the need to do any evaluating of layer-by-layer zoom styles, and between threshold zoom levels we wouldn't have to worry about checking style settings. This might speed up zooming.
Of course, we'd lose the ability to create custom zoom levels for individual layers. All layers would need to obey the global zoom style settings. So if that kind of customizability is important, we should stick with the current approach.