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

When loading KML documents we will spread out loading to prevent freezing #8195

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

tfili
Copy link
Contributor

@tfili tfili commented Sep 19, 2019

We load the first 128 features, then give up time to the render loop. This is using setTimeout with a value of 0 since we don't have a modern promise library.

Running some tests, it was slightly slower but prevented freezing. With a 51MB KMZ file the loading went from 18s to 23s, but the overall hanging went from 18s to a single 3s freeze.

@cesium-concierge
Copy link

Thanks for the pull request @tfili!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@OmarShehata
Copy link
Contributor

🎉 🎉 🎉 🎉 🎉 🎉

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

The code changes look fine to me.

Loading the KML examples in sandcastle took noticeably longer to render in this branch. I can see that this is a win for very large KML files with lots of features, but would we have a way to only do the deferred loading once the file size or number of features is larger than some threshold? Why did you pick 128? Maybe make that value larger?

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Is this meant to replace #3970?

@tfili
Copy link
Contributor Author

tfili commented Sep 20, 2019

Loading the KML examples in sandcastle took noticeably longer to render in this branch.

I didn't see that in my testing. The times were almost the same. The GDP one was even faster. The facilities was slightly slower.

@tfili
Copy link
Contributor Author

tfili commented Sep 20, 2019

Why did you pick 128? Maybe make that value larger?

Larger values made the mouse movement more choppy and smaller values didn't really help much.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

The facilities one was the one that was the most noticeable. In master it loads almost instantly. In this branch it took a few seconds.

@tfili
Copy link
Contributor Author

tfili commented Sep 20, 2019

With a build version of the app in incognito mode, in master facilities loaded in 800ms and in this branch it was 1200ms. The GDP kml was near identical (master was less than 100ms faster, which may be noise) and the bike race document was actually faster in this branch.

Upping the number from 128 will make them closer, but with larger documents it will increase jumpyness, but it will still be interactive to a point. The size of the file isn't a very good measure. A lot of documents have full HTML pages embedded in them that show up in the info bubble for a feature, which can make them giant but with only a few features and facilities is like 600KB with a crap ton of features. I guess it comes down to if we care about small documents being a little slower to load or not. I have some more mid-range sized files I can test with to see how changing that number effects them. I was testing with a pretty large document.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 20, 2019

Okay, that sounds fine to me @tfili. Thanks for taking the time to get some more numbers.

Someone else should take a quick look at this, but it's okay with me to merge =) @mramato do you have a minute to check it out?

@tfili
Copy link
Contributor Author

tfili commented Sep 20, 2019

Here is the loading time comparing master, branch with 128 entity limit and branch with 512 entity limit. The 512 one seems more jumpy and master freezes. This is the times for just the parsing of the KML file. Times don't include KMZ decompressing and creation of graphics resources. Times in ms.

File master 128 512
1.8MB KMZ (13.7MB KML) 6376 7611 5541
5MB KML 2304 3146 2533
9MB KMZ (38MB KML) 2219 3351 2174
48MB KMZ (182MB KML) 14523 16714 15400

@tfili
Copy link
Contributor Author

tfili commented Sep 23, 2019

This is updated to now use time instead of the number of features since they can vary pretty widely. It allows 1 sec to load off the bat and then follow up loading happens in 0.1 sec increments. This allows smallish documents to load as fast as they did before and larger documents to load in reasonable time.

@mramato
Copy link
Contributor

mramato commented Sep 23, 2019

Update CHANGES.md

@tfili
Copy link
Contributor Author

tfili commented Sep 23, 2019

Done

@mramato
Copy link
Contributor

mramato commented Sep 23, 2019

I think that's all I have @tfili while I still hold out hope for a shared time-slicing system in CesiumJS down the line, this is a decent incremental change and should be easy enough to tweak as needed if we see any unwanted behavior. The 1 second load before slicing seems like a great idea that should work well in practice as well.

I still wish this (optionally) tied into the render loop somehow, but that could be part of the larger time-slicing system (one day).

@mramato
Copy link
Contributor

mramato commented Sep 24, 2019

Thanks again, @tfili!

@mramato mramato merged commit 0d21bca into master Sep 24, 2019
@mramato mramato deleted the kml-defer-loading branch September 24, 2019 12:23
@OmarShehata
Copy link
Contributor

This is supposed to supersede #3970 right?

@mramato
Copy link
Contributor

mramato commented Sep 24, 2019

Yes, thanks/.

@maxvonhippel
Copy link

Was this also done for GeoJSON?

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.

6 participants