Defer processing of KML to allow for smoother performance #3970
Conversation
|
The nice thing is that this doesn't change anything functionally, so the current unit tests were an excellent 'security blanket' that I didn't break anything. I also did some of my own testing. If @tfili has any suggestions for testing, I'd be happy to write some more. |
|
I need to look a little closer at the code, but at first glance this looks great. I want to try it with some large KML to see how much it affects performance. Thanks @mmacaula! |
Source/DataSources/KmlDataSource.js
Outdated
| @@ -1947,20 +1945,52 @@ define([ | |||
| Tour : processUnsupported | |||
| }; | |||
|
|
|||
| var promisifiedFeatureTypes = { | |||
There was a problem hiding this comment.
There's no need for both promisifiedFeatureTypes and featureTypes, I would just use featureTypes and update the functions.
| var deferred = when.defer(); | ||
| var args = Array.prototype.slice.call(arguments); | ||
| var context = this; | ||
| setTimeout(function deferredFunc(){ |
There was a problem hiding this comment.
Using setTimeout here is not an ideal solution because it happens outside the requestAnimationFrame event loop (and therefore has negative impact on overall performance). In general, setTimeout should be avoided in any Cesium application for this reason.
Instead, we have DataSource.update, which is an optional function that will be called every frame during the event loop. I think what we would want is incremental loading to happen by doing a little bit of KML work in every call to update (either through time-boxing or other form of work counting). This approach may not lend itself well to the Promise-based approach (I'd have to think about it some more), but would be a much better solution (and is something we should probably due in all of our Datasources. If you add KmlDataSource.prototype.update it should automatically start getting called every frame.
Perhaps we just change the processXXX functions to always return a "next" function that we can call to continue processing. In update, we would call the next function as many times as possible in some pre-determined allotted time. I don't think it should be hard to do. What do you think?
@shunter any thoughts here?
There was a problem hiding this comment.
Hmm, I only used setTimeout because the current version of when.js in cesium didn't support setting up stuff asynchronously (at least I couldn't tell in my research). A more updated promise lib that was a+ compatible might do things in a more performant way without having to burden the render loop with non-rendering stuff. I could always try stuffing bluebird in there and profiling and comparing to when.js to see if it makes a difference. Anything to look for as a tell for setTimeout being slow?
That said, timeboxing update calls might be a big win anyway. With very large kml files, we see very long calls to update as cesium processes the initial entities produced by the giant kml we use.
I'll wait for @shunter before doing anything now.
There was a problem hiding this comment.
Please don't tie loading of data too closely to the Cesium render loop. It will break applications that shut down Cesium's render loop to avoid burning 100% CPU all the time. I don't mind if I need to do extra work in my app to make loading progress while rendering is stopped, but please think about how it's possible at least.
There was a problem hiding this comment.
@mramato I've been churning it over and I think our best bet is still to use setTimeout, at least for now. Tying it too closely with the render loop would have unintended side effects (as @kring pointed out). It will impact performance, but at the browsers discretion. In my testing, the browser still runs the render loop (and other browser stuff like mouse clicks etc) while this processing is going on.
There was a problem hiding this comment.
I've been doing more research and really diving into event loops and microtasks. There definitely are some issues with setTimeout and I'm sure you've seen that it does take longer to process now. Luckily there are some clever ways out there that let you schedule a task on the next tick of the event loop without using setTimeout.
Resources:
https://www.nczonline.net/blog/2013/07/09/the-case-for-setimmediate/
And a polyfill that we could use to replace the setTimeout call:
https://github.com/YuzuJS/setImmediate
The demo / test site is very compelling:
http://jphpsf.github.io/setImmediate-shim-demo/
As another aside, opening devtools on this github page, and typing 'setImmediate` I see that github is using what appears to be this exact polyfill (though it's minified).
Thoughts?
|
Closing since there was no response to #3970 (comment). @mmacaula thanks for putting energy into this. If you think this - or a subset of it - is ready for production, please open another pull request. Otherwise, feel free to submit an issue for future work. |
|
@pjcozzi I think it's still a worthwhile improvement, tying up the event loop is a big detriment to users. This does need to be updated though. I've been on other projects, one issues this PR had was creating promises efficiently due to the promise library being fairly old. I heard you were changing it, is it updated now? I'd be happy to re-work so I can avoid the setTimeouts. What do you think @mramato ? |
|
Yeah, I stink; sorry for not getting back to this sooner. We haven't updated promise libraries yet (though I would like to get on that soon, I started a branch but it got messy quick). As before, my main concern here is performance, I will take another look at this branch sometime this week or weekend and see if I was just being a curmudgeon the first time around. If so, we can take this as an incremental improvement until we have a better solution. |
|
My preference would be to bring in the |
|
Are any other updates going to be made to this PR? |
|
@jeremywilliams I'm planning on revisiting this PR in June in the hopes of integrating it with the new |
|
@mramato Is this still in the works? |
|
Thanks again for your contribution @mmacaula! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
@cesium-concierge stop |
|
@mramato this is the issue I was referring to today when we were talking about vector data crashing Cesium. It sounds it's an issue of just not releasing the main thread as opposed to there just not being enough memory. |
|
@tfili is going to try to look at this soon |
|
Thanks again, this was replaced with #8195 and will go out with the Oct 1 release. |
We were running into an issue with large KML files, a ~33mb KML file would cause the whole browser tab to crash. You may say, that's just too big, but it zips down to a 2.1Mb Kmz file (email me for a sample file) and I'm sure there are lots of those around. I tracked it down to all kml processing being done in single large chain of methods that didn't release the main thread.
I took advantage of the fact that load is currently promise based already and 'promisified' the
process*functions. The result is that Cesium will no longer just hang when processing large kml files.Since when.js isn't quite a+ compliant yet, and slightly slower (see #3967 ) this sacrifices some total time to process in order to not cause the browser to freeze. Once promises are upgrade, the
deferAndWrapInPromisecode can likely be removed in place of awhen.liftor other equivalent. To me, this is worthwhile for user experience but would be happy to discuss further if it's an issue.I didn't create an issue for this but can if that fits your workflow better.