Avoid sorting of features if the sort-key is constant #78
Conversation
|
Bundle size report: Size Change: +32 B
ℹ️ View Details
|
| @@ -93,7 +95,8 @@ class CircleBucket<Layer: CircleStyleLayer | HeatmapStyleLayer> implements Bucke | |||
|
|
|||
| if (!this.layers[0]._featureFilter.filter(new EvaluationParameters(this.zoom), evaluationFeature, canonical)) continue; | |||
|
|
|||
| const sortKey = circleSortKey ? | |||
| const sortKey = sortFeaturesByKey ? | |||
| // $FlowFixMe | |||
There was a problem hiding this comment.
nit: Minor annoyance, because flow does not seem to understand that circleSortKey.evaluate can never happen while circleSortKey is null, because sortFeaturesByKey can only be true if circleSortKey was overwritten earlier.
There was a problem hiding this comment.
Maybe circleSortKey && sortFeaturesByKey will do the trick?
There was a problem hiding this comment.
It would probably work, but I don't think it helps code readability. Shall I still do it?
|
Could you please prepare a test expression (in JSON GL Style?) which is failing here - to clearly demonstrating the bug, please? Would it be possible to introduce a test into CI for this use case - to avoid regression in future? I expect the same problem does not happen GL Native - as all of this is very much related to JavaScript handling of true/false/undefined - but in case there is any relation - please make a ticket proposing fix also into C/C++ parsing code. |
It's not a bug with any expression in particular, but a logic and performance issue. We found this issue when we set a Even if you just use mapbox normally, it will sort most features by mapbox sorts once while placing objects (in worker), and then (for some features) again while rendering (main-thread). It's much easier to see this bug in the debugger; but here's a hook so you can spy on all the sorting that maplibre does (without this PR). <!DOCTYPE html>
<html>
<head>
<title>Mapbox GL JS debug page</title>
<meta charset='utf-8'>
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
<link rel='stylesheet' href='http://localhost:9966/dist/maplibre-gl.css' />
<style>
body { margin: 0; padding: 0; }
html, body, #map { height: 100%; }
</style>
</head>
<body>
<div id='map'></div>
<!--
Expects that you'll be running maplibre with `yarn run start-debug`.
-->
<script src='http://localhost:9966/dist/maplibre-gl-dev.js'></script>
<script src='http://localhost:9966/debug/access_token_generated.js'></script>
<!--
We need some hooks in the worker, so here are some helpers to hook them.
URL will probably change in the future! Right now it's https://gist.github.com/JannikGM/5832e7987499f640c4fb351cbe643883
-->
<script src="https://gist.githack.com/JannikGM/5832e7987499f640c4fb351cbe643883/raw/WorkerHook.js"></script>
<script src="https://gist.githack.com/JannikGM/5832e7987499f640c4fb351cbe643883/raw/WorkerHook-console.js"></script>
<script>
// Install a hook to report new workers
workerRun(function() {
console.log(`Worker created!`);
});
// Install a hook to report `Array.sort` in worker
workerRun(function() {
const Array_prototype_sort = self.Array.prototype.sort;
self.Array.prototype.sort = function(...args) {
if (this[0] && typeof this[0] === 'object' && 'sortKey' in this[0]) {
console.log(`Worker sorting: ${this.length} items (${this.map((e) => String(e.sortKey))})`);
}
return Array_prototype_sort.call(this, ...args);
};
});
// Install a hook to report `Array.sort` on main-thread
const Array_prototype_sort = self.Array.prototype.sort;
self.Array.prototype.sort = function(...args) {
if (this[0] && typeof this[0] === 'object' && 'sortKey' in this[0]) {
console.log(`Main-Thread sorting: ${this.length} items (${this.map((e) => String(e.sortKey))})`);
}
return Array_prototype_sort.call(this, ...args);
};
var map = new maplibregl.Map({
container: 'map',
style: 'mapbox://styles/mapbox/streets-v11',
center: [-98, 38.88],
zoom: 3
});
map.on('load', function () {
// !!! Configure this !!!
// - 'symbol' sorts in worker and main-thread
// - 'circle' sorts in worker and main-thread
// - 'line' sorts in worker only
// - 'fill' sorts in worker only
// You will also see some sorting from non-selected types, because of a mapbox bug where `undefined` still sorts.
const type = 'symbol';
const demo = 2;
// Find any matching layer
var layers = map.getStyle().layers;
var layerId;
for (var i = 0; i < layers.length; i++) {
const layer = layers[i];
if (layer.type === type) {
// If the sort-key is constant (= same for all features), then all features have the same sort priority.
// Therefore, none of these demos (except 0) should require sorting.
// Because of a mapbox bug, *all* of these are sorting most features.
// Only exception: This will *not* happen for circles and symbols with `undefined` sort-key: bug not present there!
if (demo === 0) {
// This will sort with sort-key (non-constant, so actually requires sorting)
map.setLayoutProperty(layer.id, `${type}-sort-key`, ["length", ["get", "name"]]);
} else if (demo === 1) {
// This will sort with sort-key (constant undefined)
// So even if you don't do anything, it will sort
} else if (demo === 2) {
// This will sort with sort-key (constant 5)
// So even if you misconfigure your sort-key, it will sort
map.setLayoutProperty(layer.id, `${type}-sort-key`, 5);
} else if (demo === 3) {
// This will sort with sort-key (constant undefined)
// So even if you try to remove your sort-key, it will sort
map.setLayoutProperty(layer.id, `${type}-sort-key`, 5);
map.setLayoutProperty(layer.id, `${type}-sort-key`, undefined);
}
}
}
});
</script>
</body>
</html>
At least not by me / us. I doubt it's reported yet. However, I'm not sure if I'll be allowed to do this, because mapbox has recently become a competitor in the field that our company works in; my contract actually forbids this. I'll have to discuss this with my employer. We also tried to upstream different changes to mapbox last year (before they were a competitor) and it was wasting too much of our time (no/slow/unsatisfactory review).
Probably very difficult because it's not necessarily triggered by incorrect API use. Bug "1." was a trivial coding error; To avoid new errors like this, we need better review to catch logic errors. Bug "2." is a logic error / oversight, where they didn't realize that it doesn't matter what the sort-key is, if it's a constant value that's the same for all features. I did briefly talk about my mapbox-gl-js-debugger bookmarklet on Slack already, which can assist with finding such issues (by making it easier to add hooks to production websites).
I could imagine that the bug also exists there.
Our company doesn't currently use maplibre-gl-native (so I can't allocate time to it), and most of our performance issues are in maplibre-gl-js.
|
There was a problem hiding this comment.
Looks good from my non-expert-in-this-feature perspective. Please add some text to the changelog so that it will be included in the release notes. Also, do you think it would make sense to add/modify a unit test, or is this just for performance, which would make it very hard to test for it?
Done in separate commit.
No; from my previous response:
I also made recommendations how to avoid such bugs in the future.
Purely performance (and real world benefit is probably small because of much larger bottlenecks elsewhere). |
|
@JannikGM I have invited you to the devs group, so you should be able to squash-merge it yourself. Also, make sure to edit the commit message when merging, possibly copying the whole or part of the text from the top of the PR. Thanks! |
This addresses 2 related bugs:
if (expression)) to decide if sorting was necessary, the assumption was probably that the default constant value ofundefinedwould skip sorting. However, the unevaluated mapbox expression is not the valueundefined, but an expression-object describing the value and its type. So, even if the mapbox expression was a constantundefined, the check would never be true. This leads to a lot of unnecessary sorting.This was already correct for the drawing sort in circles and symbols, and this is where the
sortFeaturesByKeyvariable name originates.undefinedto skip sorting. However, we also know that the sort-key is irrelevant if it's any constant, not justundefined.Note that the second optimization would not be possible if cross-layer sorting was implemented (see mapbox/mapbox-gl-js#1349).
I'm unable to provide good performance benchmarks because of #76.