cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

Make `IPrepareRendererResources::prepareInLoadThread` asynchronous, return a Future

Open kring opened this issue 3 years ago • 1 comments

This is a PR into #554.

Previously, prepareInLoadThread was invoked in the load thread (as is hopefully obvious from the name), but it had no option to do any truly asynchronous work. Once the function returned, the load-thread part of the process was considered complete.

This PR changes the paradigm so that prepareInLoadThread now returns a Future, and the load-thread part of the process isn't complete until the Future resolves. This allows additional network requests, work in other threads, etc. without requiring the load thread to block while those other operations are in progress. For example, it could be used to dispatch part of the work to a render thread, thread pool, or game thread.

kring avatar Sep 22 '22 08:09 kring

Thanks @kring, this looks good to me, I'll merge this and #554 once we get the comment in the other PR resolved!

nithinp7 avatar Oct 14 '22 19:10 nithinp7

@kring Since some of the fixes for issues discussed in #554 are committed here, feel free to self-merge this branch into that one. I'm happy with the future based loading changes, so up to you if you want to just merge it into the other PR to be reviewed altogether when it's done - whatever is more convenient for you!

nithinp7 avatar Oct 16 '22 20:10 nithinp7

@nithinp7 I'm going to merge this into the other PR as you suggested, but it may be useful to see what's new here since you last looked. The diff is here: https://github.com/CesiumGS/cesium-native/compare/cc75a31...ffaf65b

Noteworthy changes:

  • Implemented the "async destruction complete event" for RasterOverlay. It's a little different from the Tileset one in that the user must pass an AsyncSystem (because RasterOverlay doesn't have one itself). It asserts that equivalent AsyncSystems are provided in every call.
  • Added operator== and operator!= for AsyncSystem, so the above equivalence can be determined.
  • Changed RasterOverlay::createTileProvider to return errors in its Future, rather than reporting them and returning nullptr. This allowed some code to be deleted from every raster overlay type. Load errors are reported by RasterOverlayCollection instead of by each RasterOverlay.
  • Added a new third-party library. expected-lite, which is used for the above. expected-lite is an implementation of std::expected which will be available in future C++ standards (23 I think?). It's also roughly an implementation of the Either monad from Haskell 🤓 and a nice way for a single return value to contain one set of data on success and another on failure. expected-lite is already used in at least one internal Cesium project.
  • Removed pOverlay from RasterOverlayLoadFailureDetails.
  • Fixed a couple of cases (DebugColorizeTilesRasterOverlay and RasterizedPolygonsOverlay) where an overlay was creating its tile provider with this as its parent overlay, rather than the passed-in pOwner.
  • Added debug code to ReferenceCountedNoThreadSafe to assert that it's not used from multiple threads, because that would be unsafe.

Funny story: after all the above, I was still sometimes seeing deadlocks at Tileset or RasterOverlay destruction. I poured through code looking for the reference counting error or race condition, but found nothing. I tried adding some code to help trap the condition, but it was a Heisenbug.

Eventually I got to the point where I couldn't reproduce it at all anymore and had nearly convinced myself I had imagined it, when I started doing some other stuff and it reappeared. I finally realized the cause: while waiting in IsReadyForFinishDestroy, I was dispatching main thread tasks, but I wasn't ticking the asset accessor. So, if an HTTP request was in flight when the Cesium3DTileset destruction was started, that request would never finish, and the Tileset could never be destroyed. But if all the requests were already cached, no asset accessor ticking was necessary in order for the requests to be fulfilled, and so the bug didn't appear. Basically, the more consistent I was about the steps I used to try to reproduce the bug, the less likely it was that the bug would appear. 😆 The good news: it was an easy fix once I realized what was happening. I'm just embarrassed that it took so long!

kring avatar Oct 18 '22 09:10 kring

Sounds good I'll review it there! Also didn't think I'd ever hear the word monad again 😂

Regarding the http asset accessor tick, I was looking at that IsReadyForFinishDestroy code for quite a while trying to confirm to myself that loads were progressing as they should through dispatchInMainThread - I had completely forgotten that the asset accessor was tick based! Wish I could have caught it earlier too hahaha. It might be useful to add a debug option to bypass cache at some point.

nithinp7 avatar Oct 18 '22 14:10 nithinp7