Transfer approx terrain heights to workers instead of requesting #6716
Conversation
|
@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
| if (packedLength > 0) { | ||
| var array = new Float64Array(packedLength); | ||
| subTaskTransferableObjects = [array.buffer]; | ||
| transferTerrainHeightsPromise.then(function() { |
There was a problem hiding this comment.
Note to reviewer: diff tool needs to chill here.
There was a problem hiding this comment.
Adding the query param w=1 will make the diff ignore whitespace changes
https://github.com/AnalyticalGraphicsInc/cesium/pull/6716/files?w=1
|
I think those test failures are because Now to figure out why. |
| // Push tasks transferring the JSON to the workers. | ||
| transferTerrainHeightsPromise = ApproximateTerrainHeights.initialize() | ||
| .then(function() { | ||
| var terrainHeightsString = JSON.stringify(ApproximateTerrainHeights._terrainHeights); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
…c, handle possible timeout
likangning93
force-pushed
the
transferApproxTerrainHeights
branch
from
86c94c5 to
925c011
Compare
|
I'm against this PR going into master. I think a much better approach here would be to modify If what I'm proposing can't work for some reason, then I suppose this PR might be a decent fallback, but definitely not my first choice. |
|
Replaced by #6720 (though I'm going to keep this branch around until that one gets merged). |
See discussion in #6715.
Fixes #6711