Use murmur hash on symbol instance keys to reduce worker transfer costs. #7127

ChrisLoer · 2018-08-14T19:23:17Z

Start using murmurhash-js on symbol instance "keys" for use in the cross tile symbol index. This may theoretically reduce memory consumption a little bit -- but the main goal is to be able to convert the symbolInstances array into a transferrable type to reduce worker transfer costs.

Loading some z14 LA streets, I don't see much of a difference -- I think this is because the hashed key ends up getting converted to a string that's not that different from the average length of a road name, e.g. "1239807915" vs. "Maple Ave.". So this change doesn't really buy us anything until we finish converting symbolInstances into a transferrable type.

Before	After	Total Before	Total After
74793	113346	390523	389736
103504	74568
98636	103360
113590	98462

briefly describe the changes in this PR
~~write tests for all new functionality~~
~~document any changes to public APIs~~
post benchmark scores
manually test the debug page
~~tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes~~

cc @mourner @ansis

ChrisLoer · 2018-08-16T23:55:35Z

OK, symbolInstances now gets transferred directly from the background to the foreground. There were two kind of ugly parts left behind:

collisionArrays used to live on each symbolInstance, I moved it out to be part of symbolBucket. The whole thing looks pretty weird -- what I want to eventually do is just generate the collision data on the foreground so we stop having all this weird transfer/transform logic.
crossTileID lives in a parallel (non-serialized) JS array that's part of SymbolInstanceArray.

I did a little bit of profiling, and struct array "get()" calls now show up peppered through all the placement/opacity-update code, but I'm not sure how significant it is. I think it's probably a little bit of a trade-off between CPU time spent in placement and time blocked transferring tile data. Let's just try to figure out some benchmarks that can give us a good idea of which is better?

ChrisLoer · 2018-08-17T00:04:17Z

🤷‍♂️ Maybe this is a decent win? I think my machine is just lagging overall today for me so it felt pretty stuttery but when I did 40 seconds of animation going back and forth between zooms 7 and 16 in DC, I got 53.1 FPS before the change and 56.5 FPS after.

ChrisLoer · 2018-08-17T18:08:13Z

After restarting my computer, I started getting results closer to what I'm used to, and it looks promising: as if we're roughly back to v0.45 performance after all the changes.

Each cell is number of frames generated over 10 second zoom animation, 600=60 FPS. The first entry in the table is slower because no tiles are cached (think of the first line in the table as the one that really exercises the worker transfer changes). 1071x854px viewport on 2016 MBP, location of #7/38.888/-77.019 zooming from 7 to 16 and back.

After Transferrable	Before Transferable	Before `ece1649`	v0.45
576	531	548	569
600	595	586	599
592	575	583	600
596	589	585	600

Murmur hash only added ~1.5KB to the bundle, but it looks like all the new struct array stuff added another ~5K. 😞

ansis · 2018-08-17T19:13:56Z

src/data/array_types.js

@@ -941,6 +1005,96 @@ export class PlacedSymbolArray extends StructArrayLayout2i2ui3ul3ui2f2ub40 {

 register('PlacedSymbolArray', PlacedSymbolArray);

+class SymbolInstanceStruct extends Struct {


running yarn run codegen gets rid of this. Do you have some uncommitted changes?

😳 OMG you mean I didn't have to make that all by hand. 🤦‍♂️

ChrisLoer · 2018-08-20T21:19:06Z

I modified the code-gen to add an optional "non-transferable members" argument to createStructArrayType. The idea is that a non-transferred member has no overhead in the background, but presents the same "pretend to be a plain javascript object" interface on the foreground. Right now I only support the number type, although it'd be pretty easy to add other types (just by specifying a default initializer). I'm not sure if I like how it turned out. In the end for this PR I'm just adding crossTileID, and it'd probably work just as well to explicitly generate that in the background as "0" and transfer it...

Preparation for making symbolInstances array transferrable.

ansis

In the end for this PR I'm just adding crossTileID, and it'd probably work just as well to explicitly generate that in the background as "0" and transfer it...

Do you foresee other things using this? If not, do you think it's worth removing this and just generating it in the background?

ansis · 2018-08-22T18:52:37Z

src/data/bucket/symbol_bucket.js

@@ -538,15 +518,26 @@ class SymbolBucket implements Bucket {
        arrays.programConfigurations.populatePaintArrays(arrays.layoutVertexArray.length, feature, feature.index);
    }

-    _addCollisionDebugVertex(layoutVertexArray: StructArray, collisionVertexArray: StructArray, point: Point, anchor: Point, extrude: Point) {
+    addSymbolInstance(anchorX: number, anchorY: number, horizontalPlacedTextSymbolIndex: number,


Would it make sense to just call symbolInstances.emplaceBack(...) directly instead of through this method?

This change decreases worker transfer time which should decrease jankiness during tile loading. However, there is some extra overhead in accessing the StructArray of symbolInstances during render-time symbol placement. If we identify hotspots, the most direct optimization is to used indexed accessors into the underlying TypedArray, instead of using the `get(i).property` approach.

ChrisLoer · 2018-08-22T19:46:07Z

Do you foresee other things using this? If not, do you think it's worth removing this and just generating it in the background?

When I started on the changes, there were more things that needed it, but in the end it was really just crossTileID and generating it in the background works fine -- so I updated to do that.

mourner · 2018-08-22T20:43:55Z

src/data/bucket/symbol_bucket.js

-            const aRotated = Math.round(sin * a.anchor.x + cos * a.anchor.y) | 0;
-            const bRotated = Math.round(sin * b.anchor.x + cos * b.anchor.y) | 0;
+            const a = this.symbolInstances.get(aIndex);
+            const b = this.symbolInstances.get(bIndex);


Let's make sure sorting doesn't get more expensive here — perhaps worth trying to build rotated and featureIndices arrays first and then only do (rotated[aIndex] - rotated[bIndex]) || (featureIndices[bIndex] - featureIndices[aIndex]) in the comparison function.

Good idea, I'll do that. 👍

… symbols.

ChrisLoer requested a review from mourner August 14, 2018 19:23

mourner mentioned this pull request Aug 17, 2018

Worker transfer benchmark #7145

Merged

ChrisLoer requested a review from ansis August 17, 2018 18:01

ansis reviewed Aug 17, 2018

View reviewed changes

ChrisLoer force-pushed the hash-symbol-key branch from 36d723c to 15fffa9 Compare August 20, 2018 21:12

ChrisLoer added 2 commits August 20, 2018 14:23

Use murmur hash on symbol instance keys to reduce worker transfer costs.

dd7c01c

Move from variable number of "placedTextSymbolIndices to 0,1, or 2.

57ee650

Preparation for making symbolInstances array transferrable.

ansis reviewed Aug 22, 2018

View reviewed changes

ChrisLoer force-pushed the hash-symbol-key branch from 15fffa9 to 0fd76c1 Compare August 22, 2018 19:44

ansis approved these changes Aug 22, 2018

View reviewed changes

mourner approved these changes Aug 22, 2018

View reviewed changes

Make symbolInstance lookups per SymbolBucket sort linear in number of…

9ae96a7

… symbols.

ChrisLoer merged commit 180669e into master Aug 22, 2018

ChrisLoer deleted the hash-symbol-key branch August 22, 2018 23:24

mourner mentioned this pull request Aug 28, 2018

Reevaluate worker payload and serialization #7011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use murmur hash on symbol instance keys to reduce worker transfer costs. #7127

Use murmur hash on symbol instance keys to reduce worker transfer costs. #7127

ChrisLoer commented Aug 14, 2018 •

edited

Loading

ChrisLoer commented Aug 16, 2018

ChrisLoer commented Aug 17, 2018

ChrisLoer commented Aug 17, 2018

ansis Aug 17, 2018

ChrisLoer Aug 17, 2018

ChrisLoer commented Aug 20, 2018

ansis left a comment

ansis Aug 22, 2018

ChrisLoer Aug 22, 2018

ChrisLoer commented Aug 22, 2018

mourner Aug 22, 2018

ChrisLoer Aug 22, 2018

		@@ -941,6 +1005,96 @@ export class PlacedSymbolArray extends StructArrayLayout2i2ui3ul3ui2f2ub40 {

		register('PlacedSymbolArray', PlacedSymbolArray);

		class SymbolInstanceStruct extends Struct {

Use murmur hash on symbol instance keys to reduce worker transfer costs. #7127

Use murmur hash on symbol instance keys to reduce worker transfer costs. #7127

Conversation

ChrisLoer commented Aug 14, 2018 • edited Loading

ChrisLoer commented Aug 16, 2018

ChrisLoer commented Aug 17, 2018

ChrisLoer commented Aug 17, 2018

ansis Aug 17, 2018

Choose a reason for hiding this comment

ChrisLoer Aug 17, 2018

Choose a reason for hiding this comment

ChrisLoer commented Aug 20, 2018

ansis left a comment

Choose a reason for hiding this comment

ansis Aug 22, 2018

Choose a reason for hiding this comment

ChrisLoer Aug 22, 2018

Choose a reason for hiding this comment

ChrisLoer commented Aug 22, 2018

mourner Aug 22, 2018

Choose a reason for hiding this comment

ChrisLoer Aug 22, 2018

Choose a reason for hiding this comment

ChrisLoer commented Aug 14, 2018 •

edited

Loading