CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
938 stars 295 forks source link

Remove libCurl limit on maximum HTTP requests #1292

Closed csciguy8 closed 11 months ago

csciguy8 commented 11 months ago

Set the the HttpMaxConnectionsPerServer config value to 0, to prevent limiting the number of outgoing HTTP requests to 16.

This value defaults to 16 in FHttpModule and eventually ends up setting a value in libcurl, CURLMOPT_MAX_HOST_CONNECTIONS.

Users may have not noticed this limitation in use, since instances of ACesium3DTileset default to MaximumSimultaneousTileLoads of 20, which is close enough. When using higher values however, it's very possible this is capping performance unknowingly.

To verify the effect of this value, run a Cesium Performance test with this set to 1 (Ex. Cesium.Performance.GoogleTiles.LocaleChrysler), and you should see much slower load times.

(stumbled upon this while investigating network performance)

cesium-concierge commented 11 months ago

Thanks for the pull request @csciguy8!

Reviewers, don't forget to make sure that:

kring commented 11 months ago

How do you feel about setting a higher limit, but not "unlimited"? The danger is that poorly-written systems in Unreal itself or in other plugins could queue up a very large number of simultaneous requests, and with the default setting they would proceed in an orderly fashion. But with it set to unlimited, they'd all start at once and lead to problems, such as very poor performance, unreasonable memory usage, or stalls when they all come back at once. I'm not sure how likely this is, but it seems we could get most of the benefit from setting a higher but finite limit, like 40, while preventing the most catastrophic behavior in edge cases.

csciguy8 commented 11 months ago

How do you feel about setting a higher limit, but not "unlimited"?

That's a good idea. Get out of the way of MaximumSimultaneousTileLoads, but still exercise some control of bad values.

Keep in mind the HTTP thread still limits runaway queuing of requests with RunningThreadedRequestLimit (100). I don't see any reason why we shouldn't explicitly set both of these values.

A value of 40 sounds fine. In all the performance tests I've done with MaximumSimultaneousTileLoads, I've never seen any benefit in going above 28.

Committed these changes. Ready for another look. @kring

kring commented 11 months ago

Thanks @csciguy8!