Avoid use of 'new Function' when it is prohibited by CSP

[anandthakker]

Fixes #559

Changes:

1. When new Function() is unavailable, generate StructArray accessors using closures rather than compiling strings.
2. Reinstate the specialized filter-specific expressions from Support expressions in filters #5193, and convert legacy filters into these expressions when new Function() is unavailable

Benchmarks

There's definitely a performance hit when eval is disabled. Benchmarks with significant change:

Bench	master	branch
Layout	101 ms	168 ms ( +67.6 ms / 12.8 std devs )
LayoutDDS	5.30 ms	7.01 ms ( +1.71 ms / 2.3 std devs )
Paint	14.1 ms	17.1 ms ( +2.95 ms / 2.0 std devs )
LayerLine	0.0791 ms	0.101 ms ( +0.0218 ms / 2.9 std devs )
FilterCreate	0.378 ms	1.55 ms ( +1.17 ms / 1.9 std devs )
Filter Evaluate	0.235 ms	0.606 ms ( +0.371 ms / 12.6 std devs )

All: https://bl.ocks.org/anonymous/raw/d24cf0531c261480dbdd8301c21cbd49/

Launch Checklist

• briefly describe the changes in this PR
• write tests for all new functionality
• document any changes to public APIs
• post benchmark scores
• manually test the debug page

[anandthakker]

This works, insofar as it allows GL JS to operate in the absense of the 'unsafe eval' CSP directive. My lingering questions:

1. Is the performance hit acceptable?
   Per chat w/ @ansis and @jfirebaugh, we could very likely do better on the StructArray front by switching to hard-coded layouts and using one buffer per paint property. (This would let us avoid having parallel eval/non-eval implementations, too.) It's probably something we'd like to do anyway, but it makes more sense to do this after @jfirebaugh 's PropertyValue refactor and maybe also @lbud 's state management work. Should we block the present PR until we can complete this deeper refactor?

2. How should we test this?
   I added non-eval versions of the StructArray and filter unit tests, but that doesn't guard against new Function() being introduced somewhere else (esp. through a new dependency). Ideally, it'd be great to run the integration test suite with our eval support feature detect manually disabled, but that would increase an already very long running time for tests. Maybe we could add a second container on CircleCI and run it in parallel? (Maybe this is overkill)

[jfirebaugh]

Any idea where the reduced paint performance comes from? Offhand I wouldn't expect that, since painting doesn't make much use of filters or StructArrays.

[anandthakker]

@jfirebaugh I haven't profiled yet, but I figured it was due to some of the symbol placement stuff that uses StructArrays at render time (e.g. updateLineLabels)

[anandthakker]

Here's some rough math to estimate how much filter evaluation contributes to tile processing time:

The Layout benchmark processes 65186 features in 101ms
The FilterEvaluate benchmark processes 9453 features in 0.152 ms
(0.152 / 9453) / (101 / 65186) = .010377838

So, for a style like Mapbox Streets, feature filtering accounts (very roughly) for 1% of layout time.

Thus, we'd expect the 3-4x increase in filter evaluation time caused by using expressions instead of eval to appear as a 3-4% increase in layout time; and, indeed, benchmarking just the filter change: https://bl.ocks.org/anonymous/raw/292bf0bf3cb060ec1dd92a1c1fd05d28/.

Given these figures, should we keep the eval-based path for filters, or just take the 3-4% increase in layout time in return for having a single code path?

cc @jfirebaugh @mourner

[jfirebaugh]

I'm definitely in favor of having a single code path. Let's implement the static buffer layouts and then check layout performance again -- static buffer layouts should be even faster than dynamic, eval'ed StructArray layouts, which could gain back the filter time.

[anandthakker]

Let's implement the static buffer layouts and then check layout performance again -- static buffer layouts should be even faster than dynamic, eval'ed StructArray layouts, which could gain back the filter time.

👍 agreed - I started working on this yesterday, based on your property-refactor branch.

[anandthakker]

Just rebased this branch on struct-array-refactor.

static buffer layouts should be even faster than dynamic, eval'ed StructArray layouts, which could gain back the filter time

Since that refactor didn't improve the time on the Layout benchmark, it's not surprising that it's still about the same here (https://bl.ocks.org/anonymous/raw/fe815b9cfea3c50491cb747625c34157/): +4.7ms / 5%.

Worth noting, though, that LayoutDDS is much faster under struct-array-refactor -- like 3x.

[jfirebaugh]

Current benchmark results: https://bl.ocks.org/anonymous/raw/d34a550b987e7dcb991155067c25d6d5/

They haven't changed much from #5665 (comment), but now include a 0.43.0 comparison.

[jfirebaugh]

I was able to find an optimization in earcut that counterbalances the effect of this change on the Layout benchmark.

https://bl.ocks.org/anonymous/raw/567b169f33518d7809563f2e18982137/

[zlk89]

Do we have any timeline for releasing this fix?

[anandthakker]

@zlk89 it will be included in the 0.44 release, which should be coming soon. You can watch https://github.com/mapbox/mapbox-gl-js/milestone/25 to follow progress

[nrako]

@jfirebaugh – I've noticed that the Mapbox website (how-to-use-mapbox-securely/#csp-directives) still mentions that unsafe-eval is required for the script-src directive.

[asheemmamoowala]

@Nrake, thanks for reporting the docs issue. It is being tracked at #6892