Change request queue from a heap to a sorted array

[lilleyse]

This changes the data structure that RequestScheduler uses to prioritize requests. Previously it used a heap-based priority queue. This was fine for getting the highest-priority requests, but probably more often we need to get the lowest-priority requests, such as when a request is inserted and pushes a lower priority request out of the queue.

The problem with a max-heap is you don't actually know the min value, so we were approximating the min value to be the last element in the array. While that element is definitely not the highest priority item, it is not guaranteed to be the lowest.

I went with a lower-tech solution instead and am just using a sorted array so we always know the request with the highest and lowest priorities. Yes - insertion can be O(n) instead of O(log(n)) now, but it isn't TOO bad because the 3D Tiles traversal code pushes requests in a semi-sorted order already so the average complexity is probably better than that. Plus our queue is only 20 elements at the moment.

I ran this on a few 3D Tiles datasets and saw some decent improvements with the new method:

Request priority change:

San Miguel

BEFORE
All tiles are loaded in 59.6 seconds.
Cancelled: 4491
Total: 669
Attempted: 44375

AFTER:
All tiles are loaded in 54.1 seconds.
Cancelled: 6240
Total: 670
Attempted: 45314


BEFORE no skip lods
All tiles are loaded in 76.1 seconds.
Cancelled: 4311
Total: 924
Attempted: 20054

AFTER no skip lods
All tiles are loaded in 67.0 seconds.
Cancelled: 4862
Total: 924
Attempted: 19991


Large point cloud (additive)

BEFORE:
All tiles are loaded in 22.6 seconds.
Cancelled: 1349
Total: 122
Attempted: 7195

AFTER:
All tiles are loaded in 17.9 seconds.
Cancelled: 1408
Total: 122
Attempted: 6871

• Note: a cancelled request is a request that was pushed to the queue but later removed. It's not an XHR request that was cancelled. The overhead for cancelling is low.

[cesium-concierge]

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

---

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

[pjcozzi]

• Good before/after results.
• Update CHANGES.md.
• Are you sure you want this to go right into master? Not a temporary branch? Seems OK to go into master as future optimizations probably won't impact it?
• Are you sure about the sorted array? Sounds like a lot of memory allocation and CPU burning? Given the need for constant add/remove, a (balanced) tree is generally a much better data structure than an array. Check out
  • Min-max heap
  • Which data structure to use for accessing min/max in constant-time?
  • Which data structure should I use to have fast deletion/insertion and max (or min) search?

[lilleyse]

These are the CPU results between master and this branch. I'm pretty sure this is a case where the time complexity doesn't matter much due to small-ish array size.

Sorted array (2 runs)


Heap (2 runs)

[lilleyse]

I switched the base branch to request-performance.

[pjcozzi]

I don't have enough context to digest those apples-to-apples, but are you sure this is not creating a lot of GC pressure given all the array slicing and splicing? With a tree, small object allocation is efficient AFAIK and could also be replaced with a custom allocator.

Are you also sure that this array will always be small even as we tune other things?

I would suggest one of two paths forward:

• More investigation now
• Start a public tasklist (could be another PR) for things for us to consider before request-performance is merged and then see how much this matters in light of the other optimizations.

[lilleyse]

The array and heap both have a fixed size capacity so no allocations are occurring during insertion or any of the other operations.

The main GC pressure might be the constant creation of Request objects, but this has always been the case. This happens before it even gets to the RequestScheduler.

Are you also sure that this array will always be small even as we tune other things?

That could be possible. I don't think it makes sense for the array to get too high, but I suppose it could get up to around 50 if testing shows that helps.

I'd rather move on to another area for now, so I'll open that task list with ideas.

[pjcozzi]

OK, will do a quick review now, just all re-considering this to the tasklist.

[pjcozzi]

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.

[lilleyse]

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.

[pjcozzi]

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

[pjcozzi]

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

[lilleyse]

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.

[pjcozzi]

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

[lilleyse]

All items added to the task list in #6243.

Update CHANGES.md

Same comment as #6247 (comment)

[pjcozzi]

@ggetz is this worth considering for you to work on?

[lilleyse]

This will most likely become obsolete with @kring's changes to request scheduling, so I'm going to close for now, but not delete the branch yet.