Skip to content
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.

Sign up for GitHub

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

Fix race condition causing SymbolBucket isEmpty errors #3681

Merged
merged 11 commits into from
Dec 5, 2016
Merged

Fix race condition causing SymbolBucket isEmpty errors #3681

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4cacc0e
Fix race condition causing SymbolBucket isEmpty errors
Nov 22, 2016
07c9d5a
Fix *real* race condition: don't let workerTile reparse while in the …
Nov 23, 2016
45afbc6
Add a callback to serializeBuckets to respect that it takes time; fix…
Dec 1, 2016
2c4591b
Move function declaration (lint error)
Dec 1, 2016
e3c849a
Update VectorTileWorkerSource test to reflect that WorkerTile.redoPla…
Dec 1, 2016
1959cce
Remove unnecessary callbacks added in previous commit
Dec 3, 2016
6a95dd8
Remove unnecessary redoPlacementAfterDone concept, move collisionTile…
Dec 3, 2016
29db5d7
DRY symbol bucket placement code
Dec 3, 2016
dbd4259
Create CollisionTile only after async glyph/icon returns to ensure it…
Dec 3, 2016
1158a57
Recalculate symbol layers in time for bucket.prepare
Dec 5, 2016
8e86224
Set this.angle and this.pitch in redoPlacement
Dec 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions js/source/vector_tile_worker_source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,27 @@ class VectorTileWorkerSource {
*/
reloadTile(params, callback) {
const loaded = this.loaded[params.source],
uid = params.uid;
uid = params.uid,
vtSource = this;
if (loaded && loaded[uid]) {
const workerTile = loaded[uid];
workerTile.parse(workerTile.vectorTile, this.layerIndex, this.actor, callback);

if (workerTile.status === 'parsing') {
workerTile.reloadCallback = callback;
} else if (workerTile.status === 'done') {
workerTile.parse(workerTile.vectorTile, this.layerIndex, this.actor, done.bind(workerTile));
}

}

function done(err, data) {
if (this.reloadCallback) {
const reloadCallback = this.reloadCallback;
delete this.reloadCallback;
this.parse(this.vectorTile, vtSource.layerIndex, vtSource.actor, reloadCallback);
}

callback(err, data);
}
}

Expand Down
44 changes: 21 additions & 23 deletions js/source/worker_tile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@ class WorkerTile {
this.collisionBoxArray = new CollisionBoxArray();
this.symbolInstancesArray = new SymbolInstancesArray();
this.symbolQuadsArray = new SymbolQuadsArray();
const collisionTile = new CollisionTile(this.angle, this.pitch, this.collisionBoxArray);
const sourceLayerCoder = new DictionaryCoder(Object.keys(data.layers).sort());

const featureIndex = new FeatureIndex(this.coord, this.overscaling, collisionTile, data.layers);
const featureIndex = new FeatureIndex(this.coord, this.overscaling);
featureIndex.bucketLayerIDs = {};

const buckets = {};
Expand Down Expand Up @@ -103,13 +102,11 @@ class WorkerTile {
}
}


const done = () => {
this.status = 'done';
const collisionTile = this.placeSymbols(this.showCollisionBoxes);

if (this.redoPlacementAfterDone) {
this.redoPlacement(this.angle, this.pitch, null);
this.redoPlacementAfterDone = false;
}
this.status = 'done';

const transferables = [];
callback(null, {
Expand Down Expand Up @@ -144,14 +141,9 @@ class WorkerTile {
deps++;
if (deps === 2) {
for (const bucket of this.symbolBuckets) {
// Layers are shared and may have been used by a WorkerTile with a different zoom.
for (const layer of bucket.layers) {
layer.recalculate(this.zoom);
}

bucket.prepare(stacks, icons);
bucket.place(collisionTile, this.showCollisionBoxes);
}

done();
Copy link
Contributor

@jfirebaugh jfirebaugh Dec 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to happen immediately prior to prepare as well, since prepare uses layer properties. (Making this less fragile is tracked in #3479.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
};
Expand All @@ -177,12 +169,25 @@ class WorkerTile {

redoPlacement(angle, pitch, showCollisionBoxes) {
if (this.status !== 'done') {
this.redoPlacementAfterDone = true;
this.angle = angle;
this.pitch = pitch;
return {};
}

const collisionTile = new CollisionTile(angle, pitch, this.collisionBoxArray);
const collisionTile = this.placeSymbols(showCollisionBoxes);

const transferables = [];
return {
result: {
buckets: serializeBuckets(this.symbolBuckets, transferables),
collisionTile: collisionTile.serialize(transferables)
},
transferables: transferables
};
}

placeSymbols(showCollisionBoxes) {
const collisionTile = new CollisionTile(this.angle, this.pitch, this.collisionBoxArray);

for (const bucket of this.symbolBuckets) {
// Layers are shared and may have been used by a WorkerTile with a different zoom.
Expand All @@ -193,14 +198,7 @@ class WorkerTile {
bucket.place(collisionTile, showCollisionBoxes);
}

const transferables = [];
return {
result: {
buckets: serializeBuckets(this.symbolBuckets, transferables),
collisionTile: collisionTile.serialize(transferables)
},
transferables: transferables
};
return collisionTile;
}
}

Expand Down