-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify Bucket #3465
Simplify Bucket #3465
Conversation
Trimming happens automatically in StructArray#serialize.
@@ -58,8 +58,6 @@ class Bucket { | |||
options.featureIndex.insert(feature, this.index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code isn't used for symbols. SymbolBucket
has its own definition of populate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -94,7 +94,7 @@ class WorkerTile { | |||
}); | |||
|
|||
bucket.populate(features, options); | |||
featureIndex.bucketLayerIDs[bucket.index] = family.map(getLayerId); | |||
featureIndex.bucketLayerIDs[bucket.index] = family.map((l) => l.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.concat(collisionTile_.transferables)); | ||
buckets: util.values(buckets) | ||
.filter((b) => !b.isEmpty()) | ||
.map((b) => b.serialize(transferables)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
collisionTile: collisionTile.serialize(transferables), | ||
collisionBoxArray: this.collisionBoxArray.serialize(), | ||
symbolInstancesArray: this.symbolInstancesArray.serialize(), | ||
symbolQuadsArray: this.symbolQuadsArray.serialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, why are the latter three not put into transferables? Are they already there indirectly through the symbol buckets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's an oversight from #2624.
Some relatively straightforward API simplifications for
Bucket
:getTransferables
andserialize
into a single method.Launch Checklist