Use struct array for feature id maps and deserialize on the main thread #7015
Conversation
| @@ -175,6 +176,18 @@ createStructArrayType('feature_index', createLayout([ | |||
| { type: 'Uint16', name: 'bucketIndex' } | |||
| ]), true); | |||
|
|
|||
| createStructArrayType('paint_buffer_offset', createLayout([ | |||
| // the index of the feature in the original vectortile | |||
| { type: 'Float64', name: 'featureId' }, | |||
There was a problem hiding this comment.
Corresponds to uint64_t in native
There was a problem hiding this comment.
I still confused by this. What's the underlying type defined in the vector tile spec? A Float64 can't represent every uint64_t.
There was a problem hiding this comment.
The vector tile spec doesn't define a type AFAICT. Native vt libraries use a unint64_t to represent ids. The vectortile.js library uses a number type to represent the id, which I believe has the same precision issue as Float64 does here.
There was a problem hiding this comment.
The feature IDs in the .proto file of the current vector tile spec are 64-bit unsigned integers, although this should have been mentioned in the text too. GeoJSON prefers string IDs, so for VT3 we are discussing generalizing IDs to any representable type and would appreciate input on what is feasible in client code.
There was a problem hiding this comment.
Huh yeah JS's Number.MAX_SAFE_INTEGER == 2^53 - 1 so technically that's a bug in our current implementation... although apparently we've gotten this far without hearing anyone complain about the feature ID 2^54 not working.
There was a problem hiding this comment.
@ChrisLoer that limit is for Integers. But Number is always a double, with Number.MAX_VALUE == 1.79E+308 . In order to use a large Number in a typed array, as is the case here, the array type should be large enough to accommodate uint64_t. Float64 is the only type that can do that here.
There was a problem hiding this comment.
Created #7020 to track the feature ID bit-size discrepancy.
| @@ -22,7 +22,8 @@ const AttributeType = { | |||
| Uint16: 'UNSIGNED_SHORT', | |||
| Int32: 'INT', | |||
| Uint32: 'UNSIGNED_INT', | |||
| Float32: 'FLOAT' | |||
| Float32: 'FLOAT', | |||
| Float64: 'FLOAT' | |||
There was a problem hiding this comment.
Added this for Flow. Float64 shouldn't be used for vertex buffers.
asheemmamoowala
added
the
performance ⚡
asheemmamoowala
changed the title
|
I'm not sure if this change has performance benefits. Even though it does reduce the JSON (non-TypedArray) size for higher zoom tiles, I couldnt measure or perceive improvements (less jank) in panning or zooming animations. Pan and Zoom animations over 10s both seem to drop more frames (as compared to Tested with the map centered over a dense part of NYC ( @mourner @ChrisLoer any thoughts on how else to measure this change's performance impact? |
Can you do before/after JS profiler captures of the same 10 second animation, and look for changes in time spent in Might also be interesting to look at the flame chart, find long frames, and just look for qualitative differences before/after. |
|
Closed in favor of #7132 |
Follow up from #7011. The feature id map adds significant size to the JSON transfer from WebWorkers to the main thread.
Push all feature id and paint buffer offsets into a struct array and deserialize into an object/map on the main thread.
I've introduced a
Float64type for explicit support of feature ids which areuint64_tin native.cc @ChrisLoer @ansis @mourner