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

Request Scheduler #3371

Merged
merged 2 commits into from
Jan 4, 2016
Merged

Request Scheduler #3371

merged 2 commits into from
Jan 4, 2016

Conversation

lilleyse
Copy link
Contributor

For #3241

I started off with a basic RequestScheduler that combines the code in throttleRequestByServer, RequestsByServer, and the RequestScheduler in Cesium3DTileset. The 3D Tiles code is now using the new class.

For scheduling based on priority, my initial thoughts are to collect the requests that are made during the frame, and at the end the frame sort the requests based on a sorter function (in most cases SSE based) and send the requests if available slots are open.

To Do:

  • Convert all other code that still uses throttleRequestsByServer
  • Schedule requests based on priority

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 23, 2015

For scheduling based on priority, my initial thoughts are to collect the requests that are made during the frame, and at the end the frame sort the requests based on a sorter function (in most cases SSE based) and send the requests if available slots are open.

The sort function will need to be more involved, e.g., we might want to download geometry before textures because we would rather load new buildings before texturing existing ones; we may also want to guarantee progress for both terrain and buildings even though all the building tiles are closer than the requested terrain tiles. The user should be able to configure some of this in a simple way.

Look at general scheduling algorithms like those used for processes in an OS. I did a reasonable - but perhaps incomplete job - in JobScheduler where it gives each WebGL resource type a budget to avoid starvation, but allows stealing from other budgets based on temporal coherence.

Also, as we discussed, waiting until the end of the frame could impact the tree traversal, but I think it is the only option to allow Cesium to make the system-wide "optimal" requests. So 3D Tiles may ask for 6 tiles, but only be able to request 1. Perhaps we can use temporal coherence to avoid burning CPU every frame on tree traversal when we know our budget is limited or was cut last frame due to something more important. I dunno; beware of the architect astronaut in me.

I am also happy to merge incremental improvements in multiple PRs. Once it is solid, and the rest of Cesium is also using it (terrain, imagery, glTF, and a way to explicitly include data sources and requests with our utility functions), we'll cherry-pick the commits for a PR into master.

*
* @see {@link http://wiki.commonjs.org/wiki/Promises/A|CommonJS Promises/A}
*/
var RequestScheduler = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's mark this @private until we know how it is exposed, e.g., 3D Tiles will just use it under the hood, but if a user creates a Model instance for a glTF asset, they may choose to make the request immediately or put it in the scheduler. At least, I'm pretty sure that is how we will want to do it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 23, 2015

Can you open a PR into master to deprecate throttleRequestByServer and an issue to remove it from the public API in 1.18. I suspect no one is using it outside of Cesium, and it will go away based on this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 23, 2015

Just those comments. Let me know if you want to merge this now or continue work on this branch.

@lilleyse
Copy link
Contributor Author

I replaced all areas that use throttleRequestByServer with RequestScheduler, and fixed some tests that were clogging it up with unresolved requests.

Once this is checked, we can merge this branch and do the priority scheduling work in a new PR.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 28, 2015

Looks great, except the processStateMachine tests pass in master, but fail in this branch:

image

Did you run all the tests?

@lilleyse
Copy link
Contributor Author

lilleyse commented Jan 4, 2016

I reran the tests and they are passing on my machine.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 4, 2016

They pass on my work Windows laptop, but fail on my Mac. Will show you.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 4, 2016

We're having some other issues with the Mac. Will merge for now and reevaluate in the 3d-tiles branch if needed.

pjcozzi added a commit that referenced this pull request Jan 4, 2016
@pjcozzi pjcozzi merged commit 2a00d37 into 3d-tiles Jan 4, 2016
@pjcozzi pjcozzi deleted the request-scheduler branch January 4, 2016 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants