Request Scheduler Prioritization #3397

lilleyse · 2016-01-07T14:44:35Z

Continuation of #3371 for #3241

I added support for prioritizing requests based on distance. While we want something more generic in the future, I would like some feedback on the approach I'm taking.

The request scheduler uses information about the previous frame to determine the number of requests allowed for each server during the given frame. resetBudgets will sort by distance any requests that couldn't be made last frame and estimate new budgets. When a request is made, it can give a yes/no answer by either returning a promise or undefined, as it did before. The request can be sent immediately and not defer until the end of the frame. There may be cases where the view changes drastically between frames and the estimates from the previous frame are off, but this may not be a problem. If it is, I can introduce the concept of stealing from budgets like in JobScheduler.

I had to modify a lot of functions in order to send the distance with each request. I considered generating the distance using the x, y, level, and tiling scheme instead, which would make the code cleaner but has its own limitations.

lilleyse · 2016-01-07T14:46:41Z

Specs/spec-main.js

@@ -58,6 +58,8 @@

        /*global jasmineRequire,jasmine,exports,specs*/

+        Cesium.RequestScheduler.prioritize = false;


A lot of the specs fail if prioritization is enabled because it lags behind a frame. So I added this here for simplicity sake, but it's probably a bad place...

Looks like an OK place to me, I don't think there is a beforeAll that applies to all suites.

Also, add a comment about why this line is here.

pjcozzi · 2016-01-07T17:17:15Z

I considered generating the distance using the x, y, level, and tiling scheme instead, which would make the code cleaner but has its own limitations.

This won't be general enough because, for example, we want to prioritize all requests like glTF assets and their external resources.

pjcozzi · 2016-01-07T17:40:54Z

Source/Core/ArcGisImageServerTerrainProvider.js

@@ -200,11 +202,15 @@ 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,


When this goes into master - or sooner - include all changes to the public API in CHANGES.md.

pjcozzi · 2016-01-07T18:41:37Z

It would be good to test this with something like the DC buildings and Bing imagery, and see how it compares to before 3D Tiles requests were prioritized. http://cesiumjs.org/WashingtonDC/

pjcozzi · 2016-01-07T18:50:23Z

Source/Core/RequestScheduler.js

+            return;
+        }
+
+        printStats();


Remove this. Instead, for example, the Sandcastle example could call this each frame (or it could be commented out in the Sandcastle example).

pjcozzi · 2016-01-07T18:53:57Z

That's all my comments.

lilleyse · 2016-01-13T19:32:49Z

Updated.

I created a Request class that is like the priority object mentioned in these comments, but also stores additional information about the request and is used inside RequestScheduler.

I added the ability to defer a request, so if there aren't any open slots on the current frame it will still return a promise that will be resolved once an opening appears. This is useful for all the requests that aren't imagery/terrain/3d-tiles related. I went through the codebase looking for loading functions like loadArrayBuffer, loadJson, etc and changed them to use the RequestScheduler. @mramato suggested the possibility of doing the inverse approach, and having loadWithXhr call RequestScheduler.

I haven't tested with the Washington DC buildings yet because I'm having some problems converting the tileset, but I hope to see it working soon. Also CHANGES.md still needs to be updated, but I'm waiting until this is final.

pjcozzi · 2016-01-14T15:43:42Z

@lilleyse can you merge 3d-tiles into this?

pjcozzi · 2016-01-14T15:45:15Z

@mramato suggested the possibility of doing the inverse approach, and having loadWithXhr call RequestScheduler.

This was my gut too, but I'm open to other approaches. Will have a look now.

pjcozzi · 2016-01-14T15:53:27Z

Source/Core/Request.js

+         */
+        this.distance = defaultValue(options.distance, 0.0);
+
+        // Helper members for RequestScheduler


Mark these as @private since they are being used like C#'s internal.

pjcozzi · 2016-01-14T16:11:06Z

I know this was already merged into 3d-tiles, but the implementation of RequestScheduler.hasAvailableRequests is too slow and allocates memory. This is an important function that is called on each tile during tree traversal. Perhaps we can implement some caching (that the caller could store) to avoid the memory allocation and maybe even implicit hash lookup. I dunno, it might not be perfectly straightforward since, for example, a 3D tileset can reference tiles on many different servers. Perhaps there is a fast approximation or coarse-grained check we can use.

pjcozzi · 2016-01-14T16:14:23Z

Source/Core/RequestScheduler.js

+     * @returns {Promise.<Object>} A Promise for the requested data.
+     */
+    RequestScheduler.request = function(url, requestFunction, parameters) {
+        return RequestScheduler.throttleRequest(new Request({


Perhaps this should have a type of "OTHER", "USER", "CUSTOM", or something like that, which could be useful for setting priorities and avoiding starvation.

pjcozzi · 2016-01-14T17:21:10Z

Source/Scene/Model.js

@@ -1010,7 +1014,7 @@ define([
            setCachedGltf(model, cachedGltf);
            gltfCache[cacheKey] = cachedGltf;

-            loadArrayBuffer(url, options.headers).then(function(arrayBuffer) {
+            RequestScheduler.request(url, loadArrayBuffer, options.headers).then(function(arrayBuffer) {


I think I mentioned this in a past review, but Model may be making requests on behalf of 3D Tiles so I think we should allow an override for the request type via a @private options parameter on the constructor.

pjcozzi · 2016-01-14T17:51:51Z

One simple way would be for each content provider to store its server.

Perhaps each tile would store a reference to a small object that is a new RequestsAvailable (or something like that) class. At initialization, they would ask RequestScheduler for this object based on their url, then they can check it instead of the slow path currently in RequestScheduler. Under-the-hood, RequestScheduler could do something similar.

The idea is that if we know a request won't go through this frame, we want to learn that as quickly as possible and bail on tree traversal. Likewise, we want requests that may go through to be queued up as quickly as possible with minimal scheduler overhead.

pjcozzi · 2016-01-18T13:37:21Z

Source/Scene/Cesium3DTileset.js

+            var removeFunction = removeFromProcessingQueue(tiles3D, tile);
+            when(tile.processingPromise).then(addToProcessingQueue(tiles3D, tile)).otherwise(removeFunction);
+            when(tile.readyPromise).then(removeFunction).otherwise(removeFunction);
+        }
    }

    function hasAvailableRequests(tiles3D) {


I believe this line is wrong:

return RequestScheduler.hasAvailableRequests(tiles3D._url)

This assumes that the tile's content has the same base path as tileset.json, right? They could be on two separate servers.

lilleyse · 2016-01-18T21:51:32Z

Updated.

pjcozzi · 2016-01-18T22:34:35Z

Source/Core/ArcGisImageServerTerrainProvider.js

@@ -231,7 +237,19 @@ define([
            url = proxy.getURL(url);
        }

-        var promise = RequestScheduler.throttleRequest(url, loadImage);
+        // TODO : request used to be a boolean called throttleRequest. Continue to handle that case until it is deprecated.


Remove this TODO and friends, and submit an issue for the deprecation.

pjcozzi · 2016-01-18T22:50:33Z

Just those comments.

Are you happy with how this worked for the DC dataset?

pjcozzi · 2016-01-21T13:37:54Z

Source/Scene/Cesium3DTileset.js

+        if (!tile.isContentUnloaded()) {
+            var stats = tiles3D._statistics;
+            ++stats.numberOfPendingRequests;
+            addLoadProgressEvent(tiles3D);


This call keeps track of the number of requests so it is possible that it could increase the number of pending requests here even though the scheduler doesn't make the request.

At one point we talked about adding an event (or promise or whatever) that fires when the request is actually made (or not). Perhaps we should use that to call addLoadProgressEvent.

Note that this should all happen before events are raised at the end of the frame with frameState.afterRender.

I would also be fine (perhaps even prefer) raising this event once per frame instead of per tile. The former is probably better for a UI, and the later for debugging.

lilleyse · 2016-01-22T16:26:18Z

I cleaned up the code and have been testing with the DC dataset. When testing with prioritization enabled vs. disabled, there are some small differences but its hard to say that one method is actually better than another. It's easier to notice the benefits in contrived cases where the maximum requests is set to some low number. Usually both 3D Tiles and Imagery/Terrain servers have enough open slots to do their work with or without budgeting.

I also tested the difference for when Cesium3DTileset checks if there are available requests vs. always sending requests (slower, but creates accurate budgets). Like above, its hard to see an actual visual difference. I started working on the starvation issue, but in order to do it correctly accounting for distance and the distance ranges of other budgets, it just seemed too complicated for little gain.

lilleyse · 2016-01-22T17:08:05Z

This is ready from my end.

pjcozzi · 2016-01-22T22:06:40Z

Looks good. Please cherry pick the commits for a PR into master and update the roadmap as we discussed offline.

lilleyse · 2016-01-25T13:54:49Z

Is this ready to be merged here?

pjcozzi · 2016-01-25T15:29:39Z

Yes.

Request Scheduler Prioritization

lilleyse added 2 commits January 6, 2016 15:44

Prioritize requests by distance

825689e

Pass distance to the Request Scheduler

40082fe

lilleyse reviewed Jan 7, 2016
View reviewed changes

Handle unissued requests in Cesium3DTileset

546f19d

pjcozzi reviewed Jan 7, 2016
View reviewed changes

Updated RequestScheduler

71f6c50

lilleyse force-pushed the request-scheduler branch from 2e2cc25 to 71f6c50 Compare January 13, 2016 19:07

Add doc link

712881e

pjcozzi reviewed Jan 14, 2016
View reviewed changes

Merge branch '3d-tiles' into request-scheduler

7fef79b

pjcozzi reviewed Jan 18, 2016
View reviewed changes

lilleyse added 2 commits January 18, 2016 16:47

RequestScheduler updates

a7286e6

Merge branch '3d-tiles' into request-scheduler

7805c29

pjcozzi reviewed Jan 18, 2016
View reviewed changes

RequestScheduler cleanup

c3f0326

pjcozzi reviewed Jan 21, 2016
View reviewed changes

lilleyse added 3 commits January 21, 2016 09:10

Added asset property to other test tilesets

5b4bb9a

private getter for requestServer

0d94b53

Added baseUrl to Cesium3DTileset to fix loading issues

afdebab

pjcozzi added a commit that referenced this pull request Jan 25, 2016

Merge pull request #3397 from AnalyticalGraphicsInc/request-scheduler

7e0d85a

Request Scheduler Prioritization

pjcozzi merged commit 7e0d85a into 3d-tiles Jan 25, 2016

pjcozzi deleted the request-scheduler branch January 25, 2016 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request Scheduler Prioritization #3397

Request Scheduler Prioritization #3397

lilleyse commented Jan 7, 2016

lilleyse Jan 7, 2016

pjcozzi Jan 7, 2016

pjcozzi Jan 7, 2016

pjcozzi commented Jan 7, 2016

pjcozzi Jan 7, 2016

pjcozzi commented Jan 7, 2016

pjcozzi Jan 7, 2016

pjcozzi commented Jan 7, 2016

lilleyse commented Jan 13, 2016

pjcozzi commented Jan 14, 2016

pjcozzi commented Jan 14, 2016

pjcozzi Jan 14, 2016

pjcozzi commented Jan 14, 2016

pjcozzi Jan 14, 2016

pjcozzi Jan 14, 2016

pjcozzi commented Jan 14, 2016

pjcozzi Jan 18, 2016

lilleyse commented Jan 18, 2016

pjcozzi Jan 18, 2016

pjcozzi commented Jan 18, 2016

pjcozzi Jan 21, 2016

pjcozzi Jan 21, 2016

lilleyse commented Jan 22, 2016

lilleyse commented Jan 22, 2016

pjcozzi commented Jan 22, 2016

lilleyse commented Jan 25, 2016

pjcozzi commented Jan 25, 2016

		@@ -58,6 +58,8 @@

		/global jasmineRequire,jasmine,exports,specs/

		Cesium.RequestScheduler.prioritize = false;

Request Scheduler Prioritization #3397

Request Scheduler Prioritization #3397

Conversation

lilleyse commented Jan 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 7, 2016

Choose a reason for hiding this comment

pjcozzi commented Jan 7, 2016

Choose a reason for hiding this comment

pjcozzi commented Jan 7, 2016

lilleyse commented Jan 13, 2016

pjcozzi commented Jan 14, 2016

pjcozzi commented Jan 14, 2016

Choose a reason for hiding this comment

pjcozzi commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 14, 2016

Choose a reason for hiding this comment

lilleyse commented Jan 18, 2016

Choose a reason for hiding this comment

pjcozzi commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jan 22, 2016

lilleyse commented Jan 22, 2016

pjcozzi commented Jan 22, 2016

lilleyse commented Jan 25, 2016

pjcozzi commented Jan 25, 2016