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.

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

Change request queue from a heap to a sorted array #6240

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
185 changes: 185 additions & 0 deletions Source/Core/RequestQueue.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
define([
'./Check',
'./defineProperties'
], function(
Check,
defineProperties) {
'use strict';

/**
* Priority queue for the {@link RequestScheduler} implemented as a sorted array.
* The request with the highest priority is placed at index 0 and the request
* with lowest priority is placed at index <code>length - 1</code>.
* <p>
* A lower <code>request.priority</code> value indicates that the request has higher priority. See {@link Request#priority}.
* </p>
*
* @alias RequestQueue
* @constructor
* @private
*
* @param {Number} maximumLength The maximum length of the queue.
*/
function RequestQueue(maximumLength) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.number.greaterThan('maximumLength', maximumLength, 0);
//>>includeEnd('debug');

this._array = new Array(maximumLength);
this._length = 0;
this._maximumLength = maximumLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this were public, there should be a getter for this. As private, I guess it doesn't matter.

}

defineProperties(RequestQueue.prototype, {
/**
* Gets the length of the queue.
*
* @memberof RequestQueue.prototype
*
* @type {Number}
* @readonly
*/
length : {
get : function() {
return this._length;
}
}
});

/**
* Get the request at the given index.
*
* @param {Number} index The index of the request.
*
* @return {Request} The request at the given index.
*/
RequestQueue.prototype.get = function(index) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.number.greaterThanOrEquals('index', index, 0);
Check.typeOf.number.lessThan('index', index, this._length);
//>>includeEnd('debug');
return this._array[index];
};

/**
* Insert a request into the queue. If the length would grow greater than the maximum length
* of the queue, the lowest priority request is removed and returned.
*
* @param {Request} request The request to insert.
*
* @return {Request|undefined} The request that was removed from the queue if the queue is at full capacity.
*/
RequestQueue.prototype.insert = function(request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering how and when this is called during a frame - you know better than me - does it make more sense to queue up inserts, sort them, then merge once per frame so there is potentially less overhand that constantly keeping this sorted?

Or maybe there is another variation - just take advantage of this being specific to requests, not a general min-max queue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the rationale for not queuing up all the requests is we can know right away if a request will not be able to be issued, and return undefined in RequestScheduler.request. This leads to a faster path in terrain/imagery/3d-tiles than if the request is issued but later cancelled. I'll have to think about tradeoffs between the two approaches.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that can be roadmapped as part of the optimizations.

//>>includeStart('debug', pragmas.debug);
Check.defined('request', request);
//>>includeEnd('debug');

var array = this._array;
var previousLength = this._length;
var maximumLength = this._maximumLength;

if (previousLength < maximumLength)
{
++this._length;
}

if (previousLength === 0)
{
array[0] = request;
return;
}

var removedRequest;
var lastIndex = previousLength - 1;

if (previousLength === maximumLength) {
var lastRequest = array[lastIndex];
if (request.priority >= lastRequest.priority) {
// The array is full and the priority value of this request is too high to be inserted.
return request;
}
// The array is full and the inserted request pushes off the last request
removedRequest = lastRequest;
--lastIndex;
}

while (lastIndex >= 0 && request.priority < array[lastIndex].priority) {
array[lastIndex + 1] = array[lastIndex]; // Shift element to the right
--lastIndex;
}
array[lastIndex + 1] = request;

return removedRequest;
};

/**
* Call the given function for each request in the queue.
*
* @type {RequestQueue~ForEachCallback} The function to call.
*/
RequestQueue.prototype.forEach = function(callback) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.func('callback', callback);
//>>includeEnd('debug');

var array = this._array;
var length = this._length;
for (var i = 0; i < length; ++i) {
callback(array[i]);
}
};

/**
* Sorts the queue.
*/
RequestQueue.prototype.sort = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we stay with this, is it worth extracting this out like we did for mergeSort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was about to start extracting it out but realized that this is a slightly atypical case. This class contains a fixed-sized array and a separate length variable, the idea being that we would not have to edit .length constantly. I don't think a generic insertion-sort-on-array would work with this.

var array = this._array;
var length = this._length;

// Use insertion sort since our array is small and likely to be mostly sorted already.
// Additionally length may be smaller than the array's actual length, so calling array.sort will lead to incorrect results for uninitialized values.
for (var i = 1; i < length; ++i) {
var j = i;
while ((j > 0) && (array[j - 1].priority > array[j].priority)) {
var temp = array[j - 1];
array[j - 1] = array[j];
array[j] = temp;
--j;
}
}
};

/**
* Remove <code>length</code> number of requests from the top of the queue.
*
* @param {Number} length The number of requests to remove.
*/
RequestQueue.prototype.remove = function(length) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.number.greaterThanOrEquals('length', length, 0);
Check.typeOf.number.lessThanOrEquals('length', length, this._length);
//>>includeEnd('debug');
if (length === 0) {
return;
}
if (length === this._length) {
this._length = 0;
return;
}

// Shift remaining requests back to the left
var array = this._array;
for (var i = length; i < this._length; ++i) {
array[i - length] = array[i];
}
this._length -= length;
};

/**
* The callback to use in forEach.
* @callback RequestQueue~ForEachCallback
* @param {Request} request The request.
*/

return RequestQueue;
});
82 changes: 39 additions & 43 deletions Source/Core/RequestScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ define([
'./defined',
'./defineProperties',
'./Event',
'./Heap',
'./isBlobUri',
'./isDataUri',
'./RequestQueue',
'./RequestState'
], function(
Uri,
Expand All @@ -16,16 +16,12 @@ define([
defined,
defineProperties,
Event,
Heap,
isBlobUri,
isDataUri,
RequestQueue,
RequestState) {
'use strict';

function sortRequests(a, b) {
return a.priority - b.priority;
}

var statistics = {
numberOfAttemptedRequests : 0,
numberOfActiveRequests : 0,
Expand All @@ -35,12 +31,8 @@ define([
numberOfActiveRequestsEver : 0
};

var priorityHeapLength = 20;
var requestHeap = new Heap({
comparator : sortRequests
});
requestHeap.maximumLength = priorityHeapLength;
requestHeap.reserve(priorityHeapLength);
var requestQueueLength = 20;
var requestQueue = new RequestQueue(requestQueueLength);

var activeRequests = [];
var numberOfActiveRequestsByServer = {};
Expand Down Expand Up @@ -112,29 +104,28 @@ define([
},

/**
* The maximum size of the priority heap. This limits the number of requests that are sorted by priority. Only applies to requests that are not yet active.
* The maximum length of the request queue. This limits the number of requests that are sorted by priority. Only applies to requests that are not yet active.
*
* @memberof RequestScheduler
*
* @type {Number}
* @default 20
*
* @private
*/
priorityHeapLength : {
requestQueueLength : {
get : function() {
return priorityHeapLength;
return requestQueueLength;
},
set : function(value) {
// If the new length shrinks the heap, need to cancel some of the requests.
// Since this value is not intended to be tweaked regularly it is fine to just cancel the high priority requests.
if (value < priorityHeapLength) {
while (requestHeap.length > value) {
var request = requestHeap.pop();
cancelRequest(request);
}
// Cancel all requests and resize the queue
var length = requestQueue.length;
for (var i = 0; i < length; ++i) {
var request = requestQueue.get(i);
cancelRequest(request);
}
priorityHeapLength = value;
requestHeap.maximumLength = value;
requestHeap.reserve(value);
requestQueue = new RequestQueue(value);
RequestScheduler.requestQueue = requestQueue;
}
}
});
Expand Down Expand Up @@ -242,21 +233,19 @@ define([
}
activeRequests.length -= removeCount;

// Update priority of issued requests and resort the heap
var issuedRequests = requestHeap.internalArray;
var issuedLength = requestHeap.length;
for (i = 0; i < issuedLength; ++i) {
updatePriority(issuedRequests[i]);
}
requestHeap.resort();
// Update priority of issued requests and resort the queue
requestQueue.forEach(updatePriority);
requestQueue.sort();

// Get the number of open slots and fill with the highest priority requests.
// Un-throttled requests are automatically added to activeRequests, so activeRequests.length may exceed maximumRequests
var openSlots = Math.max(RequestScheduler.maximumRequests - activeRequests.length, 0);
var filledSlots = 0;
while (filledSlots < openSlots && requestHeap.length > 0) {
// Loop until all open slots are filled or the heap becomes empty
request = requestHeap.pop();
var processedRequests = 0;
var totalRequests = requestQueue.length;
while (filledSlots < openSlots && processedRequests < totalRequests) {
// Loop until all open slots are filled or the queue becomes empty
request = requestQueue.get(processedRequests++);
if (request.cancelled) {
// Request was explicitly cancelled
cancelRequest(request);
Expand All @@ -272,6 +261,7 @@ define([
startRequest(request);
++filledSlots;
}
requestQueue.remove(processedRequests);

updateStatistics();
};
Expand Down Expand Up @@ -344,10 +334,9 @@ define([
return undefined;
}

// Insert into the priority heap and see if a request was bumped off. If this request is the lowest
// priority it will be returned.
// Insert into the priority queue and see if a request was bumped off. If this request is the lowest priority it will be returned.
updatePriority(request);
var removedRequest = requestHeap.insert(request);
var removedRequest = requestQueue.insert(request);

if (defined(removedRequest)) {
if (removedRequest === request) {
Expand Down Expand Up @@ -397,12 +386,19 @@ define([
* @private
*/
RequestScheduler.clearForSpecs = function() {
while (requestHeap.length > 0) {
var request = requestHeap.pop();
var request;
var length;
var i;

length = requestQueue.length;
for (i = 0; i < length; ++i) {
request = requestQueue.get(i);
cancelRequest(request);
}
var length = activeRequests.length;
for (var i = 0; i < length; ++i) {
requestQueue.remove(length);

length = activeRequests.length;
for (i = 0; i < length; ++i) {
cancelRequest(activeRequests[i]);
}
activeRequests.length = 0;
Expand Down Expand Up @@ -431,7 +427,7 @@ define([
*
* @private
*/
RequestScheduler.requestHeap = requestHeap;
RequestScheduler.requestQueue = requestQueue;

return RequestScheduler;
});
Loading