Simplify Bucket

[jfirebaugh]

Some relatively straightforward API simplifications for Bucket:

• Combine getTransferables and serialize into a single method.
• Eliminate explicit array trimming -- this happens automatically as part of serialization.

benchmark	master b9d9509	simplify-bucket b75bbf4
map-load	164 ms	96 ms
style-load	106 ms	102 ms
buffer	1,058 ms	1,080 ms
fps	60 fps	60 fps
frame-duration	7 ms, 0% > 16ms	7.1 ms, 0% > 16ms
query-point	1.06 ms	1.41 ms
query-box	68.11 ms	72.10 ms
geojson-setdata-small	9 ms	8 ms
geojson-setdata-large	304 ms	286 ms

Launch Checklist

• briefly describe the changes in this PR
• post benchmark scores
• manually test the debug page

[mb12]

@jfirebaugh Can you please clarify how this works correctly for symbols? Should featureIndex only have geometries that are actually rendered on the map? Would this (or not) add all point geometries as long as they match the filter?

[jfirebaugh]

This code isn't used for symbols. SymbolBucket has its own definition of populate.

[mb12]

@jfirebaugh Is it possible to point out where the insertion into feature index happens for symbols in SymbolBucket and/or how do geometries for symbol features make into the index used for queryRenderedFeatures?

The reason I am asking you this is because in an older version of GL JS, this insertion used to happen in WorkerTile and it was explicitly disabled for symbols. The only way to work around it and get symbols returned by queryRenderedFeatures was to comment out this condition.

[jfirebaugh]

Feature querying for symbols does not use FeatureIndex. Instead it uses the CollisionTile generated during placement. If you're seeing issues with symbol querying, please file a separate issue.

[mourner]

This is inside a double-loop and the closure here doesn't need any of the outer scope — I'm not sure V8 optimizes cases like this well enough, so I'd keep it a top-level function.

[jfirebaugh]

The benchmarks don't show any meaningful performance difference.

Written as is

benchmark	master b9d9509	simplify-bucket b75bbf4
buffer	919 ms	914 ms

Variant 1

function serializeBuckets(buckets, transferables) {
    return buckets
        .filter((b) => !b.isEmpty())
        .map((b) => b.serialize(transferables));
}

benchmark	master b9d9509	simplify-bucket b75bbf4
buffer	906 ms	911 ms

Variant 2

function bucketNonEmpty(bucket) {
    return !bucket.isEmpty();
}

function serializeBuckets(buckets, transferables) {
    return buckets
        .filter(bucketNonEmpty)
        .map((b) => b.serialize(transferables));
}

benchmark	master b9d9509	simplify-bucket b75bbf4
buffer	893 ms	900 ms

You can see that all the alternatives are within the range of the control (master b9d9509) -- ~890 - 920ms.

Variant 1 does avoid repeating the filter/map pipeline, so I'm inclined to go with it.

[jfirebaugh]

Here's replacing (l) => l.id with top level getLayerID (also no measurable difference):

benchmark	master b9d9509	simplify-bucket b75bbf4
buffer	887 ms	908 ms

[mourner]

Same here.

[mourner]

Quick question, why are the latter three not put into transferables? Are they already there indirectly through the symbol buckets?

[jfirebaugh]

I believe it's an oversight from #2624.