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

Transfer approx terrain heights to workers instead of requesting #6716

Closed
wants to merge 4 commits into from
Closed

Transfer approx terrain heights to workers instead of requesting #6716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f79da82
transfer approximate terrain heights JSON to workers
likangning93 Jun 21, 2018
dc1abee
revert asynchronous worker changes from fe4849e
likangning93 Jun 21, 2018
103a75a
make sure terrain heights loaded on workers for PolylineVisualizerSpec
likangning93 Jun 21, 2018
925c011
make sure terrain heights loaded on workers for GeometryVisualizerSpe…
likangning93 Jun 22, 2018
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
22 changes: 17 additions & 5 deletions Source/Core/ApproximateTerrainHeights.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ define([
'./Ellipsoid',
'./GeographicTilingScheme',
'./Rectangle',
'./Resource'
'./Resource',
'../ThirdParty/when'
], function(
buildModuleUrl,
defaultValue,
Expand All @@ -25,7 +26,8 @@ define([
Ellipsoid,
GeographicTilingScheme,
Rectangle,
Resource) {
Resource,
when) {
'use strict';

var scratchDiagonalCartesianNE = new Cartesian3();
Expand All @@ -49,20 +51,30 @@ define([
* Initializes the minimum and maximum terrain heights
* @return {Promise}
*/
ApproximateTerrainHeights.initialize = function(url) {
ApproximateTerrainHeights.initialize = function() {
var initPromise = ApproximateTerrainHeights._initPromise;
if (defined(initPromise)) {
return initPromise;
}

url = defaultValue(url, 'Assets/approximateTerrainHeights.json');
ApproximateTerrainHeights._initPromise = Resource.fetchJson(buildModuleUrl(url)).then(function(json) {
ApproximateTerrainHeights._initPromise = Resource.fetchJson(buildModuleUrl('Assets/approximateTerrainHeights.json')).then(function(json) {
ApproximateTerrainHeights._terrainHeights = json;
});

return ApproximateTerrainHeights._initPromise;
};

/**
* Initializes the minimum and maximum terrain heights from a string.
* Used by workers to receive terrain heights from the main thread.
*
* @param {String} terrainHeightsString Stringified terrain heights JSON.
*/
ApproximateTerrainHeights.initializeFromString = function(terrainHeightsString) {
ApproximateTerrainHeights._terrainHeights = JSON.parse(terrainHeightsString);
ApproximateTerrainHeights._initPromise = when();
};

/**
* Computes the minimum and maximum terrain heights for a given rectangle
* @param {Rectangle} rectangle THe bounding rectangle
Expand Down
15 changes: 0 additions & 15 deletions Source/Scene/GroundPolylinePrimitive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,21 +375,6 @@ define([
return GroundPolylinePrimitive._initialized;
};

// For use with web workers.
GroundPolylinePrimitive._initializeTerrainHeightsWorker = function() {
var initPromise = GroundPolylinePrimitive._initPromise;
if (defined(initPromise)) {
return initPromise;
}

GroundPolylinePrimitive._initPromise = ApproximateTerrainHeights.initialize('../Assets/approximateTerrainHeights.json')
.then(function() {
GroundPolylinePrimitive._initialized = true;
});

return GroundPolylinePrimitive._initPromise;
};

function createShaderProgram(groundPolylinePrimitive, frameState, appearance) {
var context = frameState.context;
var primitive = groundPolylinePrimitive._primitive;
Expand Down
92 changes: 58 additions & 34 deletions Source/Scene/Primitive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
define([
'../Core/ApproximateTerrainHeights',
'../Core/BoundingSphere',
'../Core/Cartesian2',
'../Core/Cartesian3',
Expand Down Expand Up @@ -42,6 +43,7 @@ define([
'./SceneMode',
'./ShadowMode'
], function(
ApproximateTerrainHeights,
BoundingSphere,
Cartesian2,
Cartesian3,
Expand Down Expand Up @@ -1119,57 +1121,79 @@ define([
});
}

var transferTerrainHeightsPromise;
if (!defined(createGeometryTaskProcessors)) {
createGeometryTaskProcessors = new Array(numberOfCreationWorkers);
for (i = 0; i < numberOfCreationWorkers; i++) {
createGeometryTaskProcessors[i] = new TaskProcessor('createGeometry', Number.POSITIVE_INFINITY);
}
// Push tasks transferring the JSON to the workers.
transferTerrainHeightsPromise = ApproximateTerrainHeights.initialize()
.then(function() {
var terrainHeightsString = JSON.stringify(ApproximateTerrainHeights._terrainHeights);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is a good idea, did we time this? I’m not sure why we didn’t just make everything work on the worker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not terrible, even on my chromebook-grade machine this is typically under 50 ms according to simple console.time wrapping. I think it's swallowed by the rest of worker spinup, if that's anywhere near as slow as it is in Node. Keep in mind that the JSON parse on the worker side is something that has to happen even if the worker's requesting the terrain heights instead.

We could also hold the string on the main thread alongside the parsed version, then release it after workers are spun up... I'll go ahead and add that right after I get CI passing on Travis again.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you only timed the Stringify, then you are missing the part where it gets sent over to the worker. For a string, that's probably not a lot of overhead though. However, why wouldn't just making getModuleUrl work in workers be a better solution? I feel like this another bug like this will pop up again in the future the next time we want to do something with a url on a worker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, why wouldn't just making getModuleUrl work in workers be a better solution? I feel like this another bug like this will pop up again in the future the next time we want to do something with a url on a worker.

I guess this would also get us around the test failure that I keep running into on Travis in this PR...

var workerTerrainHeightsPromises = new Array(numberOfCreationWorkers);
for (i = 0; i < numberOfCreationWorkers; i++) {
workerTerrainHeightsPromises[i] = createGeometryTaskProcessors[i].scheduleTask({
subTasks : [
{
moduleName : 'parseTerrainHeights',
geometry : terrainHeightsString
}
]
});
}
return when.all(workerTerrainHeightsPromises);
});
} else {
transferTerrainHeightsPromise = when();
}

var subTask;
subTasks = subdivideArray(subTasks, numberOfCreationWorkers);

for (i = 0; i < subTasks.length; i++) {
var packedLength = 0;
var workerSubTasks = subTasks[i];
var workerSubTasksLength = workerSubTasks.length;
for (j = 0; j < workerSubTasksLength; ++j) {
subTask = workerSubTasks[j];
geometry = subTask.geometry;
if (defined(geometry.constructor.pack)) {
subTask.offset = packedLength;
packedLength += defaultValue(geometry.constructor.packedLength, geometry.packedLength);
}
}

var subTaskTransferableObjects;

if (packedLength > 0) {
var array = new Float64Array(packedLength);
subTaskTransferableObjects = [array.buffer];
transferTerrainHeightsPromise.then(function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: diff tool needs to chill here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the query param w=1 will make the diff ignore whitespace changes
https://github.com/AnalyticalGraphicsInc/cesium/pull/6716/files?w=1

var subTask;
subTasks = subdivideArray(subTasks, numberOfCreationWorkers);

for (i = 0; i < subTasks.length; i++) {
var packedLength = 0;
var workerSubTasks = subTasks[i];
var workerSubTasksLength = workerSubTasks.length;
for (j = 0; j < workerSubTasksLength; ++j) {
subTask = workerSubTasks[j];
geometry = subTask.geometry;
if (defined(geometry.constructor.pack)) {
geometry.constructor.pack(geometry, array, subTask.offset);
subTask.geometry = array;
subTask.offset = packedLength;
packedLength += defaultValue(geometry.constructor.packedLength, geometry.packedLength);
}
}
}

promises.push(createGeometryTaskProcessors[i].scheduleTask({
subTasks : subTasks[i]
}, subTaskTransferableObjects));
}
var subTaskTransferableObjects;

primitive._state = PrimitiveState.CREATING;
if (packedLength > 0) {
var array = new Float64Array(packedLength);
subTaskTransferableObjects = [array.buffer];

when.all(promises, function(results) {
primitive._createGeometryResults = results;
primitive._state = PrimitiveState.CREATED;
}).otherwise(function(error) {
setReady(primitive, frameState, PrimitiveState.FAILED, error);
for (j = 0; j < workerSubTasksLength; ++j) {
subTask = workerSubTasks[j];
geometry = subTask.geometry;
if (defined(geometry.constructor.pack)) {
geometry.constructor.pack(geometry, array, subTask.offset);
subTask.geometry = array;
}
}
}

promises.push(createGeometryTaskProcessors[i].scheduleTask({
subTasks : subTasks[i]
}, subTaskTransferableObjects));
}

primitive._state = PrimitiveState.CREATING;

when.all(promises, function(results) {
primitive._createGeometryResults = results;
primitive._state = PrimitiveState.CREATED;
}).otherwise(function(error) {
setReady(primitive, frameState, PrimitiveState.FAILED, error);
});
});
} else if (primitive._state === PrimitiveState.CREATED) {
var transferableObjects = [];
Expand Down
12 changes: 4 additions & 8 deletions Source/Workers/createGeometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
define([
'../Core/defined',
'../Scene/PrimitivePipeline',
'../ThirdParty/when',
'./createTaskProcessorWorker',
'require'
], function(
defined,
PrimitivePipeline,
when,
createTaskProcessorWorker,
require) {
'use strict';
Expand Down Expand Up @@ -35,7 +33,7 @@ define([
function createGeometry(parameters, transferableObjects) {
var subTasks = parameters.subTasks;
var length = subTasks.length;
var resultsOrPromises = new Array(length);
var results = new Array(length);

for (var i = 0; i < length; i++) {
var task = subTasks[i];
Expand All @@ -44,16 +42,14 @@ define([

if (defined(moduleName)) {
var createFunction = getModule(moduleName);
resultsOrPromises[i] = createFunction(geometry, task.offset);
results[i] = createFunction(geometry, task.offset);
} else {
//Already created geometry
resultsOrPromises[i] = geometry;
results[i] = geometry;
}
}

return when.all(resultsOrPromises, function(results) {
return PrimitivePipeline.packCreateGeometryResults(results, transferableObjects);
});
return PrimitivePipeline.packCreateGeometryResults(results, transferableObjects);
}

return createTaskProcessorWorker(createGeometry);
Expand Down
17 changes: 6 additions & 11 deletions Source/Workers/createGroundPolylineGeometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
define([
'../Core/defined',
'../Core/GroundPolylineGeometry',
'../Scene/GroundPolylinePrimitive'
'../Core/GroundPolylineGeometry'
], function(
defined,
GroundPolylineGeometry,
GroundPolylinePrimitive) {
GroundPolylineGeometry) {
'use strict';

function createGroundPolylineGeometry(groundPolylineGeometry, offset) {
return GroundPolylinePrimitive._initializeTerrainHeightsWorker()
.then(function() {
if (defined(offset)) {
groundPolylineGeometry = GroundPolylineGeometry.unpack(groundPolylineGeometry, offset);
}
return GroundPolylineGeometry.createGeometry(groundPolylineGeometry);
});
if (defined(offset)) {
groundPolylineGeometry = GroundPolylineGeometry.unpack(groundPolylineGeometry, offset);
}
return GroundPolylineGeometry.createGeometry(groundPolylineGeometry);
}

return createGroundPolylineGeometry;
Expand Down
95 changes: 40 additions & 55 deletions Source/Workers/createTaskProcessorWorker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,13 @@
define([
'../ThirdParty/when',
'../Core/defaultValue',
'../Core/defined',
'../Core/formatError'
], function(
when,
defaultValue,
defined,
formatError) {
'use strict';

// createXXXGeometry functions may return Geometry or a Promise that resolves to Geometry
// if the function requires access to ApproximateTerrainHeights.
// For fully synchronous functions, just wrapping the function call in a `when` Promise doesn't
// handle errors correctly, hence try-catch
function callAndWrap(workerFunction, parameters, transferableObjects) {
var resultOrPromise;
try {
resultOrPromise = workerFunction(parameters, transferableObjects);
return resultOrPromise; // errors handled by Promise
} catch (e) {
return when.reject(e);
}
}

/**
* Creates an adapter function to allow a calculation function to operate as a Web Worker,
* paired with TaskProcessor, to receive tasks and return results.
Expand Down Expand Up @@ -51,53 +35,54 @@ define([
*/
function createTaskProcessorWorker(workerFunction) {
var postMessage;
var transferableObjects = [];
var responseMessage = {
id : undefined,
result : undefined,
error : undefined
};

return function(event) {
/*global self*/
var data = event.data;

var transferableObjects = [];
var responseMessage = {
id : data.id,
result : undefined,
error : undefined
};
transferableObjects.length = 0;
responseMessage.id = data.id;
responseMessage.error = undefined;
responseMessage.result = undefined;

try {
responseMessage.result = workerFunction(data.parameters, transferableObjects);
} catch (e) {
if (e instanceof Error) {
// Errors can't be posted in a message, copy the properties
responseMessage.error = {
name : e.name,
message : e.message,
stack : e.stack
};
} else {
responseMessage.error = e;
}
}

return when(callAndWrap(workerFunction, data.parameters, transferableObjects))
.then(function(result) {
responseMessage.result = result;
})
.otherwise(function(e) {
if (e instanceof Error) {
// Errors can't be posted in a message, copy the properties
responseMessage.error = {
name : e.name,
message : e.message,
stack : e.stack
};
} else {
responseMessage.error = e;
}
})
.always(function() {
if (!defined(postMessage)) {
postMessage = defaultValue(self.webkitPostMessage, self.postMessage);
}
if (!defined(postMessage)) {
postMessage = defaultValue(self.webkitPostMessage, self.postMessage);
}

if (!data.canTransferArrayBuffer) {
transferableObjects.length = 0;
}
if (!data.canTransferArrayBuffer) {
transferableObjects.length = 0;
}

try {
postMessage(responseMessage, transferableObjects);
} catch (e) {
// something went wrong trying to post the message, post a simpler
// error that we can be sure will be cloneable
responseMessage.result = undefined;
responseMessage.error = 'postMessage failed with error: ' + formatError(e) + '\n with responseMessage: ' + JSON.stringify(responseMessage);
postMessage(responseMessage);
}
});
try {
postMessage(responseMessage, transferableObjects);
} catch (e) {
// something went wrong trying to post the message, post a simpler
// error that we can be sure will be cloneable
responseMessage.result = undefined;
responseMessage.error = 'postMessage failed with error: ' + formatError(e) + '\n with responseMessage: ' + JSON.stringify(responseMessage);
postMessage(responseMessage);
}
};
}

Expand Down
Loading