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

Implement Staged Tile Loading Pipeline #779

Open
wants to merge 220 commits into
base: main
Choose a base branch
from
Open

Conversation

csciguy8
Copy link
Contributor

@csciguy8 csciguy8 commented Dec 15, 2023

When loading tiles, implement a new staged pipeline where network requests are dispatched separately from processing work.

This achieves a 25-32% loading time reduction in some test cases.

Closes #746. Also closes #473 as a side effect.

Performance Tests Summary (Cesium for Unreal Performance Tests)

SampleLocaleDenver (cold cache) - 21% slow down
SampleLocaleDenver (warm cache) -15% slow down
SampleLocaleMelbourne (cold cache) - 5% improvement
SampleLocaleMelbourne (warm cache) - about the same
GoogleTiles.LocaleDeathValley (cold cache) - 32% improvement
GoogleTiles.LocaleDeathValley (warm cache) - 15% slow down
GoogleTiles.LocaleChrysler (cold cache) - 25% improvement
GoogleTiles.LocaleChrysler (warm cache) - 9% slow down
Complete Testing Data
cesium-native main
--------------------------------------------------------------------------
SampleLocaleDenver (27 MB)
	Cold cache - 2.02, 2.03, 2.08 | 2.96, 2.45, 2.09, 2.15 - 2.04 avg (best 3)
	Warm cache - 0.81, 0.90, 0.91 | 0.98, 0.98, 0.93, 0.94 - 0.87 avg (best 3)
SampleLocaleMelbourne (89 MB)
	Cold cache - 3.38, 3.38, 3.45 | 3.54, 4.10, 4.26, 3.54 - 3.40 avg (best 3)
	Warm cache - 1.75, 1.76, 1.81 | 1.96, 2.09, 1.81, 1.83 - 1.77 avg (best 3)
GoogleTiles.LocaleDeathValley (33 MB)
	Cold cache - 4.63, 4.75, 4.91 | 5.17, 5.08, 4.96, 4.96 - 4.76 avg (best 3)
	Warm cache - 1.92, 1.96, 1.98 | 5.07, 2.00, 2.03, 2.02 - 1.95 avg (best 3)
GoogleTiles.LocaleChrysler (62 MB)
	Cold cache - 6.69, 6.84, 6.84 | 7.04, 7.09, 6.98, 6.96 - 6.79 avg (best 3)
	Warm cache - 3.06, 3.16, 3.21 | 3.62, 3.26, 3.25, 3.26 - 3.14 avg (best 3)

this PR
--------------------------------------------------------------------------
SampleLocaleDenver (27 MB)
	Cold cache - 2.43, 2.46, 2.52 | 2.99, 2.86, 2.77, 3.05 - 2.47 avg (best 3)
	Warm cache - 0.97, 1.01, 1.04 | 1.12, 1.08, 1.09, 1.05 - 1.00 avg (best 3)
SampleLocaleMelbourne (89 MB)
	Cold cache - 3.19, 3.25, 3.27 | 4.14, 5.17, 3.31, 3.84 - 3.23 avg (best 3)
	Warm cache - 1.78, 1.81, 1.88 | 1.94, 2.03, 2.06, 1.95 - 1.82 avg (best 3)
GoogleTiles.LocaleDeathValley (33 MB)
	Cold cache - 3.22, 3.24, 3.24 | 3.25, 3.32, 3.38, 3.57 - 3.23 avg (best 3)
	Warm cache - 2.24, 2.25, 2.26 | 2.28, 2.29, 2.37, 2.38 - 2.25 avg (best 3)
GoogleTiles.LocaleChrysler (62 MB)
	Cold cache - 5.05, 5.16, 5.17 | 5.20, 5.20, 5.37, 5.17 - 5.12 avg (best 3)
	Warm cache - 3.39, 3.42, 3.47 | 3.51, 3.53, 3.60, 3.57 - 3.42 avg (best 3)

In Depth

...

Simplified view of existing tile loads
image

This PR introduces this structure
image

The best place to start looking at the code changes would be in Tileset:: _processWorkerThreadLoadQueue.

Previous load requests were handled immediately, but now they are now passed to TilesetContentManager, which transforms the requests into new data that TileWorkManager can work with, a generalized TileLoadWork. TileWorkManager handles the critical logic to queue / throttle / and execute TileLoadWork objects.

TO DO

  • Fix TileLoadWork parent / child queuing problem
  • Solve shutdown problems (Ex. performance test timing out)
  • Look for any C++ modernization opportunities
  • Fixup all unit tests
  • Self review, remove any unnecessary changes
  • Merge with latest
  • Fixup last failing unit test (TestSubtreeAvailability)
  • Self review, resolve any remaining TODOs
  • Test tilesets in Unreal samples
  • Test tilesets in Unity samples
  • Run all performance tests (Unreal). Post results here
  • Investigate Google P3DT warm cache slowdown
  • Peer review / approval
  • Test ion token becoming invalid mid session
  • Test level sequences (Unreal movie render queue)
  • Verify no slowdowns in most common cases (Cesium World Terrain, Bing Maps)
  • Merge

- Change _maxSimultaneousRequests to 28 for testing
- Put loadProgress calculation into ViewUpdateResult instead of being determined on the fly
- Put loadProgress kick hack back in for testing
Add assertion to view results
Rename some members
This was just a test. In practice, the view can change frequently. We want to drop the work that doesn't make it.
No reason to bump this. Latest tests show minimal improvement
@csciguy8 csciguy8 marked this pull request as ready for review March 4, 2024 20:29
@csciguy8

This comment was marked as outdated.

…processLoadRequests)

Previously this was only happening in dispatchMainThreadTasks, at the beginning of update_view
- Handle completed work for newly dispatched work that completes immediately (like unit tests)
- Add done loading notify for tiles that fail too (but don't count towards used bytes
@csciguy8

This comment was marked as outdated.

@csciguy8
Copy link
Contributor Author

Another update to the performance test numbers...

Good news: the performance wins from this PR went up! (now 25-32% improvements for the google tests)

Mediocre news: although the inexplicable "warm cache is really slow" problem is gone, warm cache results are still slightly slower in this PR (9-21% slower). Denver cold cache falls into this category too. I have a pretty good reason to believe that the problem is here, which coincides with @kring 's recommendation to look out for main thread continuations.

The fix would be to dispatch processing work immediately, rather than waiting to dispatch on the main thread. Not a trivial change, but not an unreasonable amount of work either.

A snippet

SampleLocaleDenver (cold cache) - 21% slow down
SampleLocaleDenver (warm cache) -15% slow down
SampleLocaleMelbourne (cold cache) - 5% improvement
SampleLocaleMelbourne (warm cache) - about the same
GoogleTiles.LocaleDeathValley (cold cache) - 32% improvement
GoogleTiles.LocaleDeathValley (warm cache) - 15% slow down
GoogleTiles.LocaleChrysler (cold cache) - 25% improvement
GoogleTiles.LocaleChrysler (warm cache) - 9% slow down

@kring

This comment was marked as outdated.

Comment on lines +275 to +276
// A response code of 0 is not a valid HTTP code
// and probably indicates a non-network error.
Copy link
Member

Choose a reason for hiding this comment

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

No, it probably indicates a valid response from a file:/// URL. It's annoying that libcurl returns a status code of 0 for a successful file read, but it does.

@csciguy8

This comment was marked as outdated.

@kring

This comment was marked as outdated.

@kring

This comment was marked as outdated.

…een released

When unloading tiles, make sure raster mapped tiles aren't loading, if so, wait for them to finish. This case can happen when moving very quickly through the world, where a tile is unloaded before it is finished loading.
In LayerJsonTerrainLoader::getLoadWork, when no quadtree tile ID is detected, don't skip adding all work, just the url request work.
@csciguy8

This comment was marked as outdated.

@electrum-bowie
Copy link

Any progress on this ?

I can't wait for these improvements to come !

@kring
Copy link
Member

kring commented Jul 26, 2024

This is on hold for the time being, due to other higher-priority work and because the performance we saw with this approach was a bit mixed (some things got notably faster, but others got slower).

@electrum-bowie
Copy link

Is there any other work being done for performance ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants