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

Jump to bottom

Request Scheduler from 3d-tiles #3476

Merged
merged 15 commits into from
Jun 9, 2017
Merged

Request Scheduler from 3d-tiles #3476

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
be9e9ce
New request scheduler
lilleyse May 19, 2017
3f38546
Fix mistake from my testing
lilleyse Jun 2, 2017
a1cc29b
More tests and slightly different order-of-operations for cancelled r…
lilleyse Jun 2, 2017
172da97
More tests and fix re-sorting priority
lilleyse Jun 2, 2017
8d698e9
Fix GEE
lilleyse Jun 6, 2017
28c03c1
Spec fixes
lilleyse Jun 6, 2017
38b9d55
Fixed tests by letting unfinished tests go through
lilleyse Jun 6, 2017
da3420d
Remove RequestScheduler.clearForSpecs from being called for every test
lilleyse Jun 6, 2017
b9e8042
Cleanup
lilleyse Jun 6, 2017
0b3b1cc
Improved doc
lilleyse Jun 8, 2017
bfb356c
Merge branch 'master' into request-scheduler-master
lilleyse Jun 8, 2017
cfe237f
Clear request scheduler before each ImageryProvider test
lilleyse Jun 8, 2017
228bb06
Fix for tests
lilleyse Jun 8, 2017
40b9293
Remove while(true) which fixes eslint error
lilleyse Jun 8, 2017
bf9119e
Merge branch 'master' into request-scheduler-master
pjcozzi Jun 9, 2017
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
4 changes: 4 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ Change Log

* Deprecated
* `GoogleEarthImageryProvider` has been deprecated and will be removed in Cesium 1.37, use `GoogleEarthEnterpriseMapsProvider` instead.
* The `throttleRequest` parameter for `TerrainProvider.requestTileGeometry`, `CesiumTerrainProvider.requestTileGeometry`, `VRTheWorldTerrainProvider.requestTileGeometry`, and `EllipsoidTerrainProvider.requestTileGeometry` is deprecated and will be replaced with an optional `Request` object. The `throttleRequests` parameter will be removed in 1.37. Instead to throttle requests set the request's `throttle` property to `true`.
* The ability to provide a Promise for the `options.url` parameter of `loadWithXhr` and for the `url` parameter of `loadArrayBuffer`, `loadBlob`, `loadImageViaBlob`, `loadText`, `loadJson`, `loadXML`, `loadImage`, `loadCRN`, `loadKTX`, and `loadCubeMap` is deprecated. This will be removed in 1.37, instead `url` must be a string.

* Added an `options.request` parameter to `loadWithXhr` and a `request` parameter to `loadArrayBuffer`, `loadBlob`, `loadImageViaBlob`, `loadText`, `loadJson`, `loadJsonp`, `loadXML`, `loadImageFromTypedArray`, `loadImage`, `loadCRN`, and `loadKTX`.
* Fixed bug where if polylines were set to follow the surface of an undefined globe, Cesium would crash [#5413] https://github.com/AnalyticalGraphicsInc/cesium/pull/5413
* Fixed a bug where picking clusters would return undefined instead of a list of the clustered entities. [#5286](https://github.com/AnalyticalGraphicsInc/cesium/issues/5286)
* Reduced the amount of Sun bloom post-process effect near the horizon. [#5381](https://github.com/AnalyticalGraphicsInc/cesium/issues/5381)
Expand Down
41 changes: 22 additions & 19 deletions Source/Core/CesiumTerrainProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ define([
'./defaultValue',
'./defined',
'./defineProperties',
'./deprecationWarning',
'./DeveloperError',
'./Event',
'./GeographicTilingScheme',
Expand All @@ -19,8 +20,9 @@ define([
'./Math',
'./OrientedBoundingBox',
'./QuantizedMeshTerrainData',
'./Request',
'./RequestType',
'./TerrainProvider',
'./throttleRequestByServer',
'./TileAvailability',
'./TileProviderError'
], function(
Expand All @@ -32,6 +34,7 @@ define([
defaultValue,
defined,
defineProperties,
deprecationWarning,
DeveloperError,
Event,
GeographicTilingScheme,
Expand All @@ -43,14 +46,15 @@ define([
CesiumMath,
OrientedBoundingBox,
QuantizedMeshTerrainData,
Request,
RequestType,
TerrainProvider,
throttleRequestByServer,
TileAvailability,
TileProviderError) {
'use strict';

/**
* A {@link TerrainProvider} that access terrain data in a Cesium terrain format.
* A {@link TerrainProvider} that accesses terrain data in a Cesium terrain format.
* The format is described on the
* {@link https://github.com/AnalyticalGraphicsInc/cesium/wiki/Cesium-Terrain-Server|Cesium wiki}.
*
Expand Down Expand Up @@ -489,17 +493,16 @@ define([
* @param {Number} x The X coordinate of the tile for which to request geometry.
* @param {Number} y The Y coordinate of the tile for which to request geometry.
* @param {Number} level The level of the tile for which to request geometry.
* @param {Boolean} [throttleRequests=true] True if the number of simultaneous requests should be limited,
* or false if the request should be initiated regardless of the number of requests
* already in progress.
* @param {Request} [request] The request object. Intended for internal use only.
*
* @returns {Promise.<TerrainData>|undefined} A promise for the requested geometry. If this method
* returns undefined instead of a promise, it is an indication that too many requests are already
* pending and the request will be retried later.
*
* @exception {DeveloperError} This function must not be called before {@link CesiumTerrainProvider#ready}
* returns true.
*/
CesiumTerrainProvider.prototype.requestTileGeometry = function(x, y, level, throttleRequests) {
CesiumTerrainProvider.prototype.requestTileGeometry = function(x, y, level, request) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now every terrain/imagery provider sets throttleByServer to true which limits to 6 active requests per server. Once more of these servers switch over to http2 this flag will hold them back. With CesiumTerrainProvider and some others we'll know when the make the switch, but this doesn't help people using custom providers. Should an option like throttleByServer be exposed on the provider level for users to tweak?

This same idea applies to 3D tilesets too. A throttleByServer option could be sent in when constructing the tileset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think something less coarse-grained or at least named differently needs to be exposed. Have you looked at other engines using HTTP2 or doc on best practices with HTTP2?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've been pretty clear that I'm squarely in the "don't throttle" crowd (I'm all for prioritization and cancellation, just not throttling) so take this with that bias up front.

Just because a server supports HTTP/2, doesn't mean the client does. So it's impossible for the developer to make this decision during development. As far as I know, it's also impossible to detect at runtime whether you're using HTTP/2, so we can't do some magic on our end either. That means the best option is to not throttle at all and let the browser handle it.

I tried to hunt around to see what other webmaps are doing, and as far as I can tell none of them throttle (I checked OL/Leaflet/Mapbox). OpenLayers has an open issue to cancel unneeded requests, but that's it.

But since I'm not going to be able to convince you guys to not do it, I think the second best option is having a single top-level setting on RequestScheduler to disable it. I know the initial implementation had this, I'm not sure about the one in this branch (haven't gotten to that part of the code yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If HTTP/2 becomes widely adopted, I agree that throttling by server is a waste. However as of now throttling does make a big difference to performance:

I just got these numbers for an Everest static view:

4G with cache disabled:
  Throttled:
     9.70 seconds
     256 requests
  Not throttled:
     15.17 seconds
     412 requests

Normal usage
  Throttled
    1.45 seconds
    526 requests
  Not throttled
    1.60 seconds
    526 requests

When would the client not support HTTP/2?

Copy link
Contributor

Choose a reason for hiding this comment

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

When would the client not support HTTP/2?

Up until recently, not all browsers supported it, and even then there could be lots of atypical browsers (embedded, webview, etc..) that don't support it. In the long run, everyone will.

I just got these numbers for an Everest static view:

I know I'm missing something, but why are there less requests with the 4G version? Shouldn't the total number of requests be the same? (if we're just talking about throttling and not cancellation).

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that none of us individually knows enough about everything - tile selection and HTTP2 - to make a good call here. Let's get together for a few minutes today in person. @lilleyse please organize.

//>>includeStart('debug', pragmas.debug)
if (!this._ready) {
throw new DeveloperError('requestTileGeometry must not be called before the terrain provider is ready.');
Expand All @@ -522,8 +525,6 @@ define([
url = proxy.getURL(url);
}

var promise;

var extensionList = [];
if (this._requestVertexNormals && this._hasVertexNormals) {
extensionList.push(this._littleEndianExtensionSize ? 'octvertexnormals' : 'vertexnormals');
Expand All @@ -532,17 +533,19 @@ define([
extensionList.push('watermask');
}

function tileLoader(tileUrl) {
return loadArrayBuffer(tileUrl, getRequestHeader(extensionList));
if (typeof request === 'boolean') {
deprecationWarning('throttleRequests', 'The throttleRequest parameter for requestTileGeometry was deprecated in Cesium 1.35. It will be removed in 1.37.');
request = new Request({
throttle : request,
throttleByServer : request,
type : RequestType.TERRAIN
});
}
throttleRequests = defaultValue(throttleRequests, true);
if (throttleRequests) {
promise = throttleRequestByServer(url, tileLoader);
if (!defined(promise)) {
return undefined;
}
} else {
promise = tileLoader(url);

var promise = loadArrayBuffer(url, getRequestHeader(extensionList), request);

if (!defined(promise)) {
return undefined;
}

var that = this;
Expand Down
7 changes: 3 additions & 4 deletions Source/Core/EllipsoidTerrainProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,13 @@ define([
* @param {Number} x The X coordinate of the tile for which to request geometry.
* @param {Number} y The Y coordinate of the tile for which to request geometry.
* @param {Number} level The level of the tile for which to request geometry.
* @param {Boolean} [throttleRequests=true] True if the number of simultaneous requests should be limited,
* or false if the request should be initiated regardless of the number of requests
* already in progress.
* @param {Request} [request] The request object. Intended for internal use only.
*
* @returns {Promise.<TerrainData>|undefined} A promise for the requested geometry. If this method
* returns undefined instead of a promise, it is an indication that too many requests are already
* pending and the request will be retried later.
*/
EllipsoidTerrainProvider.prototype.requestTileGeometry = function(x, y, level, throttleRequests) {
EllipsoidTerrainProvider.prototype.requestTileGeometry = function(x, y, level, request) {
var width = 16;
var height = 16;
return new HeightmapTerrainData({
Expand Down
65 changes: 35 additions & 30 deletions Source/Core/GoogleEarthEnterpriseMetadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ define([
'./joinUrls',
'./loadArrayBuffer',
'./Math',
'./Request',
'./RequestType',
'./RuntimeError',
'./TaskProcessor',
'./throttleRequestByServer'
'./TaskProcessor'
], function(
dbrootParser,
when,
Expand All @@ -30,9 +31,10 @@ define([
joinUrls,
loadArrayBuffer,
CesiumMath,
Request,
RequestType,
RuntimeError,
TaskProcessor,
throttleRequestByServer) {
TaskProcessor) {
'use strict';

function stringToBuffer(str) {
Expand Down Expand Up @@ -130,7 +132,7 @@ define([
var that = this;
this._readyPromise = requestDbRoot(this)
.then(function() {
return that.getQuadTreePacket('', that._quadPacketVersion, false);
return that.getQuadTreePacket('', that._quadPacketVersion);
})
.then(function() {
return true;
Expand Down Expand Up @@ -292,30 +294,23 @@ define([
*
* @param {String} [quadKey=''] The quadkey to retrieve the packet for.
* @param {Number} [version=1] The cnode version to be used in the request.
* @param {Boolean} [throttle=true] True if the number of simultaneous requests should be limited,
* or false if the request should be initiated regardless of the number of requests
* already in progress.
* @param {Request} [request] The request object. Intended for internal use only.
*
* @private
*/
GoogleEarthEnterpriseMetadata.prototype.getQuadTreePacket = function(quadKey, version, throttle) {
GoogleEarthEnterpriseMetadata.prototype.getQuadTreePacket = function(quadKey, version, request) {
version = defaultValue(version, 1);
quadKey = defaultValue(quadKey, '');
throttle = defaultValue(throttle, true);
var url = getMetadataUrl(this, quadKey, version);
var proxy = this._proxy;
if (defined(proxy)) {
url = proxy.getURL(url);
}

var promise;
if (throttle) {
promise = throttleRequestByServer(url, loadArrayBuffer);
if (!defined(promise)) {
return undefined;
}
} else {
promise = loadArrayBuffer(url);
var promise = loadArrayBuffer(url, undefined, request);

if (!defined(promise)) {
return undefined; // Throttled
}

var tileInfo = this._tileInfo;
Expand Down Expand Up @@ -378,21 +373,18 @@ define([
* @param {Number} x The tile X coordinate.
* @param {Number} y The tile Y coordinate.
* @param {Number} level The tile level.
* @param {Boolean} [throttle=true] True if the number of simultaneous requests should be limited,
* or false if the request should be initiated regardless of the number of requests
* already in progress.
* @param {Request} [request] The request object. Intended for internal use only.
*
* @returns {Promise<GoogleEarthEnterpriseTileInformation>} A promise that resolves to the tile info for the requested quad key
*
* @private
*/
GoogleEarthEnterpriseMetadata.prototype.populateSubtree = function(x, y, level, throttle) {
throttle = defaultValue(throttle, true);
GoogleEarthEnterpriseMetadata.prototype.populateSubtree = function(x, y, level, request) {
var quadkey = GoogleEarthEnterpriseMetadata.tileXYToQuadKey(x, y, level);
return populateSubtree(this, quadkey, throttle);
return populateSubtree(this, quadkey, request);
};

function populateSubtree(that, quadKey, throttle) {
function populateSubtree(that, quadKey, request) {
var tileInfo = that._tileInfo;
var q = quadKey;
var t = tileInfo[q];
Expand All @@ -406,13 +398,20 @@ define([
t = tileInfo[q];
}

var subtreeRequest;
var subtreePromises = that._subtreePromises;
var promise = subtreePromises[q];
if (defined(promise)) {
return promise
.then(function() {
// Recursively call this incase we need multiple subtree requests
return populateSubtree(that, quadKey, throttle);
// Recursively call this in case we need multiple subtree requests
subtreeRequest = new Request({
throttle : request.throttle,
throttleByServer : request.throttleByServer,
type : request.type,
priorityFunction : request.priorityFunction
});
return populateSubtree(that, quadKey, subtreeRequest);
});
}

Expand All @@ -427,16 +426,22 @@ define([
// We need to split up the promise here because when will execute syncronously if getQuadTreePacket
// is already resolved (like in the tests), so subtreePromises will never get cleared out.
// Only the initial request will also remove the promise from subtreePromises.
promise = that.getQuadTreePacket(q, t.cnodeVersion, throttle);
promise = that.getQuadTreePacket(q, t.cnodeVersion, request);
if (!defined(promise)) {
return undefined;
}
subtreePromises[q] = promise;

return promise
.then(function() {
// Recursively call this incase we need multiple subtree requests
return populateSubtree(that, quadKey, throttle);
// Recursively call this in case we need multiple subtree requests
subtreeRequest = new Request({
throttle : request.throttle,
throttleByServer : request.throttleByServer,
type : request.type,
priorityFunction : request.priorityFunction
});
return populateSubtree(that, quadKey, subtreeRequest);
})
.always(function() {
delete subtreePromises[q];
Expand Down
Loading