Mapbox-gl-js: Data-Join method performance regression between 0.42.0 and 0.44.1

[tomgrek]

We're joining a 5M geojson with ~300K features to a map layer. Performance was fine (and still is fine in Safari) but with the recent Chrome 65 update has become a disaster in terms of memory and CPU usage. Memory consumption sometimes grows so high (3G+) that the Chrome tab crashes; even if it doesn't crash the app becomes severely unresponsive to the point of unusable. It's the same with Chromium.

I've included a couple of screenshots from profiling (a few seconds of panning around the map after our features have rendered) for info but let me know if I can help further.

Expected Behavior

Map rendering performs just as well as in Chrome 64 and below.

[mourner]

Hmm, interesting... Thanks for the report! Would you be able to set up a test case so that we could replicate the issue?

[anandthakker]

Additionally, it would be helpful to know what version of Mapbox GL JS you're using, and whether you see the same performance issue in previous versions. (Probably easier to answer using an isolated test case.)

[tomgrek]

Thanks for your speedy replies. Sorry -- this is with 0.44.1.

Here's an example (forked from one of your own Ryan Baumann). Load this in Chrome, especially, go to the full screen version, pan and zoom around a bit... you'll see it perform badly and eventually crash Chrome (65).

Open it in Safari, it works OK.

[jfirebaugh]

The example crashed the tab for me in Chrome 64 (64.0.3282.186) as well.

[tomgrek]

Sorry guys, do I feel like a buckethead now.

The Chrome update coincided with a teammate updating from 0.42.0 to 0.44.1, which I had not noticed. Revert to 0.42.0 and performance is back to normal.

You can also see the change in performance, here's the same Block but using 0.42 instead of 0.44 which works fine.

I guess you will look into performance in due course; I'll close this issue now.

[mourner]

This is still a valid performance concern that's not captured elsewhere, so let's investigate and keep it open.

[ryanbaumann]

I'm able to replicate this issue both Firefox v59 and Chrome v65. Thanks for the examples @tomgrek.

Given that the data-join technique (matching local data to a vector tile property using a match expression or categorical function type) is a very common for developers, we should work on this performance regression.

Symptoms I'm noticing:

• The inital map load time is not much different from 0.42 to 0.44.1
• The `performance regression is really apparant when zooming to a new level and requesting new vector tiles. In 0.44.1, there is a massive calculation going on for each new tile that arrives. In 0.41, I can zoom and pan to new tiles without any percieved difference in performance

Here's the Chrome performance profile from panning and zooming the same data-join map example with 31k matches, just changing between GL JS versions:

0.44.1 (slow)

parseCSSColor is taking up all of the CPU time. There seems to be a memory leak, too, which helps explain why the crashing symptoms in the example on this issue
profile-slow-datajoin-0.44.1.zip
.

0.41.0 (fast)
bufferData is where most time is spent, which makes sense. No memory leak.

[mourner]

The offending commit is this one https://github.com/mapbox/mapbox-gl-js/commit/ce87f20abb7847832f9a13fc9408be652f6320b3

After it, as far as I understand, expressions and old-school property functions get recreated on the main thread on every tile load for the buckets it contains. Functions in the data-join case are very expensive to create (in this particular example, a 300k color stops function), but previously it was all happening only on the web workers side, and now it blocks the main thread while seemingly being unnecessary.

To confirm this, replace this function with return null — the example continues to work while being snappy again:

https://github.com/mapbox/mapbox-gl-js/blob/3c07b72a59d78bc6240a8e3d77a54bec7619af5d/src/style-spec/expression/index.js#L258-L260

@anandthakker assigning to you since I lack the full context of why we need this deserialization.

I'll also separately look at csscolorparse module performance for the RGBA case because it looks suspiciously slow.

[anandthakker]

previously it was all happening only on the web workers side, and now it blocks the main thread while seemingly being unnecessary

This is technically is accomplishing something that _is_ necessary: because we need the expressions held by the ProgramConfigurations for a loaded tile on the main thread to be consistent with those that were used during layout on the worker. Since properties might have been changed via setPaintProperty, etc., we need to store the expression that was used for a tile with that tile.

We currently do this in the way that is simplest from a state management perspective (i.e., just store the expression directly on the ProgramConfiguration), but it incurs the overhead @mourner describes above. To avoid this, we'd need to do something like store the current set of expressions on the main thread's tile when we call loadTile, omit the *Binder#expression property from serialization, and then re-wire *Binder#expression using the stored expressions after deserializing.

[anandthakker]

To avoid this, we'd need to do something like store the current set of expressions on the main thread's tile when we call loadTile, omit the *Binder#expression property from serialization, and then re-wire *Binder#expression using the stored expressions after deserializing.

I think solving this issue the right way is really dependent on https://github.com/mapbox/mapbox-gl-js/issues/4012.

[manassra]

Hey folks! I am also facing a similar problem. I am joining values for 200k features locally with a vector tile layer that hosts the geometries (using a match expression). Chrome either crashes when I do the join, and if it doesn't, user interaction is extremely laggy (especially when zooming in/out). @ryanbaumann recommended that I downgrade to an earlier version, and I can confirm that the performance using version 0.42 is significantly better.

[asheemmamoowala]

@tomgrek with the changes in #6853 it should be possible to use feature-state expressions and Map#setFeatureState to get better performance from your expressions.

Here's an example from @ryanbaumann: https://bl.ocks.org/ryanbaumann/733ba99c5ca1d9d15259081b395e4b00