asset accessor that uses libcurl directly #634

timoore · 2024-01-25T19:25:02Z

This PR replaces the HttpAccessAccessor with the accessor from vsgCs, called UrlAssetAccessor. UrlAssetAccessor caches the CURL handle between requests, allowing libcurl to keep connections open to remote servers. This can be a big performance gain, especially for https connections. Apparently this was always problematic in the cpr library used by HttpAccessAccessor.

There was also some cleanup in the rest of the code to refer to CesiumAsync:IAssetAccessor instead of a concrete accessor type where possible.

First step to possibly replacing HttpAssetAccessor

Very little of the code needs to know the concrete type of the asset accessor. This is preparation for swapping HttpAssetAccessor for UrlAssetAccessor.

The motivation for this change is to take advantage of reusing the CURL handles, rather than tearing them down and creating them anew for each request. This allows libcurl to keep a connection to a remote server open and reuse it for multiple requests. This avoids renegotating SSL among other things and can be a big performance win.

This appears to be unnecessary when using the C interface to libcurl, as opposed to cpr. The path to the certificate is still stored, but will go away at a later time.

weegeekps

Looks good, but lacks support for proxies, which I suspect will be an issue for academic and government customers.

There is a great overview on everything.curl.dev regarding supporting proxies. Curl actually does most of the heavy lifting here, but the hard part is actually gathering the correct settings for the proxy.

I want to add, I'm not entirely sure this is required. I believe that libcpr does give us proxy detection but I can't fully remember and if it doesn't then nothing is broken by us swapping to this.

weegeekps · 2024-01-25T20:44:08Z

src/core/src/UrlAssetAccessor.cpp

+}
+
+curl_slist*
+UrlAssetAccessor::setCommonOptions(CURL* curl, const std::string& url, const CesiumAsync::HttpHeaders& headers) {


Below are a couple other easy opts that may be useful for performance. Admittedly two of these are taken from the Performance page on everything.curl.dev but in my experience these are the most impactful performance tweaks.

CURLOPT_BUFFERSIZE controls the preferred size of the receive buffer. The default (usually CURLOPT_MAX_WRITE_SIZE which is usually 16 KiB) is good enough for 90% of use cases, but for larger data like ours it is probably helpful to have it larger. My gut says 512 KiB or 1 MiB, but we may need to do a little testing here. Beware, set it once per handle in the pool, and never set it again, especially if that handle is currently being used for a connection. Bad things will happen.

CURLOPT_MAXCONNECTS defaults to 5 and controls the maximum number of connections in the connection pool. Raising this can be beneficial when you've got lots of parallel requests happening, but it can also hurt when you start considering HTTP2/3. It probably wouldn't hurt us to raise it slightly. Multi connects defaults to CURLOPT_MAXCONNECTS * 4 so that might be a good starting place.

CURLOPT_DNS_CACHE_TIMEOUT controls how long before libcurl requests the domain again. The interface libcurl uses to get DNS info doesn't include TTL, so libcurl gives all DNS entries in the cache a TTL of 60 seconds. This is way too low. Should be at least 300 seconds minimum. Higher is better (most DNS servers report 3600 seconds) but comes with caveats that if the DNS server starts reporting something else, the cache could hurt us.

Below are a couple other easy opts that may be useful for performance. Admittedly two of these are taken from the Performance page on everything.curl.dev but in my experience these are the most impactful performance tweaks.

CURLOPT_BUFFERSIZE controls the preferred size of the receive buffer. The default (usually CURLOPT_MAX_WRITE_SIZE which is usually 16 KiB) is good enough for 90% of use cases, but for larger data like ours it is probably helpful to have it larger. My gut says 512 KiB or 1 MiB, but we may need to do a little testing here. Beware, set it once per handle in the pool, and never set it again, especially if that handle is currently being used for a connection. Bad things will happen.

Do you know if that's the same as CPR_RESERVE_SIZE? That's set to 3Mb. I will set that.
I'll add the other options.

I believe CPR_RESERVE_SIZE is specific to CPR. That said, I haven't checked but I wouldn't be surprised if under the covers CPR just passes the value to libcurl using this opt.

weegeekps · 2024-01-25T20:45:16Z

src/core/src/UrlAssetAccessor.cpp

+        auto request = std::make_shared<UrlAssetRequest>(verb, url, headers);
+        auto payloadCopy = std::make_shared<std::vector<std::byte>>(contentPayload.begin(), contentPayload.end());
+        asyncSystem.runInWorkerThread([promise, request, payloadCopy, this]() {
+            VSGCS_ZONESCOPEDN("UrlAssetAccessor::request inner");


I'm guessing this is leftover tracy stuff from vsgcs? Maybe remove it?

Yes, #defined out, but still cruft. These were good tracing points in vsgCs; I'll see about replacing them with cesium-native tracing calls.

weegeekps · 2024-01-25T21:55:04Z

So it turns out CPR doesn't autoresolve proxies like I thought it did, so my comments about the proxy support are moot. Probably needs to be fixed one day.

Add back the buffersize option from HttpAssetAccessor and also options suggested in the review. Replace vsgCs tracing macro with Cesium Native tracing macro.

lilleyse · 2024-01-26T18:22:13Z

I'm seeing SSL errors on Windows:

2024-01-26 18:20:32 [24,003ms] [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.916] [cesium-omniverse] [error] [RasterOverlayCollection.cpp:127] Error while creating tile provider: curl: SSL peer certificate or SSH remote key was not OK

2024-01-26 18:20:32 [24,005ms] [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.918] [cesium-omniverse] [error] Error while creating tile provider: curl: SSL peer certificate or SSH remote key was not OK

2024-01-26 18:20:32 [Error] [cesium.omniverse.plugin] [2024-01-26 13:20:32.922] [cesium-omniverse] [error] [TilesetContentManager.cpp:825] An unexpected error occurred when loading tile: curl: SSL peer certificate or SSH remote key was not OK

timoore · 2024-01-26T19:13:06Z

I'm seeing SSL errors on Windows:

I've seen reports of this on Windows with vsgCs, but it has always worked for me. I think it's sensitive to the exact version of curl+ssl that is downloaded. I will check if cpr has some explicit dependency on SSL / TLS that isn't being satisfied now.

Test to see if fixes Windows problem. If it does, that is probably a symptom of other configuration issues.

timoore · 2024-01-26T19:47:09Z

Sean, can you try it on Windows with the latest commit? We thought that the local certificate was redundant, but maybe there are additional hoops to jump through to use OS provided certificates.

I would try it myself, but I'm in the airport and don't have any Windows environment installed.

lilleyse · 2024-01-26T20:35:46Z

The latest commit fixes it.

It would still be good to know why it's needed for Windows. I was hoping we could get away without shipping certs.

timoore · 2024-01-26T20:53:12Z

https://stackoverflow.com/questions/37551409/configure-curl-to-use-default-system-cert-store-on-windows looks promising.

weegeekps · 2024-01-26T21:10:48Z

Starting with curl 8.2.0 (July 19 2023) (issue), there is the option --ca-native

I wonder how that option translates from CLI to libcurl?

Hopefully this will remove the dependency on a local certificate.

timoore · 2024-01-29T18:37:34Z

Another try, this time with the CURLSSOPT_NATIVE_CA option. @lilleyse can you give the latest a quick try?

lilleyse · 2024-01-29T20:35:52Z

@timoore looks like there's a merge conflict now that #626 is merged. I think the strategy would be to take the changes from main and re-do the smaller changes that you made in Context, etc.

Also, could you remove HttpAssetAccessor and anything that mentions cacert.pem including the update_certs script? We don't need to install the requests library any more either. That can be removed from the developer setup and main.yml.

r-veenstra · 2024-01-30T08:14:09Z

I did some load time testing on this branch and observed some very positive results compared to current main, and also the version of main prior to this branch fork (to ensure the massive refactor didn't impact anything)

Process:

USDA with a fixed camera position over Manhattan & Google 3D tiles
Hacked in a load timer to the Cesium Debugging window so Reload all Tilesets would trigger a timer that incremented as the tiles load
Verified each test was loading the exact same quantity of tiles (2382) and fetching data across the network (~100mb for each reload)
Using release builds to ensure no debug shenanigans were at play

Current main: f89fa4e
Test 1: 92.85
Test 2: 85.75
Test 3: 87.22
Test 4: 88.51
Test 5: 86.78
Average: 88.222

This Branch: e602ddd
Test 1: 52.79
Test 2: 51.25
Test 3: 52.5
Test 4: 51.07
Test 5: 51.7
Average: 51.862

Main pre-libcurl updates: d1b7ebc
Test 1: 89.67
Test 2: 86.9
Test 3: 88.7
Test 4: 86.89
Test 5: 86.52
Average: 87.736

Unless I've made a mistake, that's a 35% load time reduction for a complex Google 3D tiles scene.

Change getHttpAssetAccessor() to getAssetAccessor(). Remove the HttpAssetAccessor source and header files and any references to them. Remove any references to the cacert.pem certificate as well as scripts and libraries used to fetch it.

lilleyse · 2024-01-31T23:01:08Z

src/core/src/Context.cpp

-const std::filesystem::path& Context::getCertificatePath() const {
-    return _certificatePath;
-}


Remove getCertificatePath from Context.h

lilleyse · 2024-01-31T23:01:36Z

src/core/include/cesium/omniverse/UrlAssetAccessor.h

+};
+
+// Simple implementation of AssetAcessor that can make network and local requests
+class UrlAssetAccessor : public CesiumAsync::IAssetAccessor {


UrlAssetAccessor, UrlAssetResponse, UrlAssetRequest could all be marked final

lilleyse · 2024-01-31T23:01:52Z

src/core/include/cesium/omniverse/UrlAssetAccessor.h

+
+    void tick() noexcept override;
+    CurlCache curlCache;
+    std::string userAgent;


Can userAgent be private?

lilleyse · 2024-01-31T23:02:52Z

src/core/include/cesium/omniverse/UrlAssetAccessor.h

+        CacheEntry()
+            : curl(nullptr)
+            , free(false) {}
+        CacheEntry(CURL* in_curl, bool in_free)


In cesium-native I see the convention free_ a lot

I prefer that too and will make the change.

lilleyse · 2024-01-31T23:08:17Z

src/core/include/cesium/omniverse/UrlAssetAccessor.h

+        bool free;
+    };
+    std::mutex cacheMutex;
+    std::vector<CacheEntry> cache;


Do the CURL objects need to be explicitly freed somewhere? Like when CurlCache is destroyed?

They don't need to be freed for the duration of the program. It's complicated to figure out the multi-threading implications of when a CURL object is in use or may be freed, and this seems to be a weakness in the curl design. I thought it was best to blow off freeing the CURL objects and just let them be deleted when the program stops.

lilleyse · 2024-01-31T23:10:11Z

src/core/src/UrlAssetAccessor.cpp

+#include <cstring>
+
+namespace cesium::omniverse {
+const auto CURL_BUFFERSIZE = 3145728L; // 3 MiB


Do you know when the buffer gets freed? Is it when curl_easy_reset is called, or when the CURL object is destroyed? Hopefully not the former.

Also I think this could be shrunk to 0.5 MiB given the avg tile sizes in CWT and Google.

I believe curl_easy_reset() doesn't do much except to prepare for another request. For example, it doesn't tear down an existing connection. The buffer freeing should probably be verified, either by examining the code or with a debugger.

lilleyse · 2024-01-31T23:11:56Z

src/core/src/UrlAssetAccessor.cpp

+    }
+
+    const std::string& method() const override {
+        return this->_method;


Style nitpick: convention in this repo is to omit this->. Though looks like native is different.

I prefer to omit this-> too. I probably copied the code directly from cesium-unreal.

lilleyse · 2024-01-31T23:15:15Z

src/core/src/UrlAssetAccessor.cpp

+        asyncSystem.runInWorkerThread([promise, request, this]() {
+            CESIUM_TRACE("UrlAssetAccessor::get");
+            CurlHandle curl(this);
+            curl_slist* list = setCommonOptions(curl(), request->url(), request->headers());


What's the overhead of calling setCommonOptions? Many of the options won't change per-request. Could some of them be set when acquiring a new handle?

If there's minimal overhead then no need to change it.

I believe that it is harmless to set the options each time and that it's not too obvious from the docs what options persist after the curl_easy_reset() call. Merits further examination.

Also, move CurlHandle out of public UrlAssetAccesor.h

lilleyse · 2024-02-13T16:44:37Z

@timoore could you update CHANGES.md summarizing the performance improvements that Ryan noted: #634 (comment)

lilleyse

Looks good. Just update CHANGES.md, ThirdParty.extra.json, and copyrights, and then you can go ahead and merge.

Update ThirdParty.extra.json.

timoore added 4 commits January 25, 2024 19:44

Initial import of vsgCs UrlAssetAccessor

b3314b6

First step to possibly replacing HttpAssetAccessor

Replace most references to HttpAssetAccessor with IAssetAccessor

a366c30

Very little of the code needs to know the concrete type of the asset accessor. This is preparation for swapping HttpAssetAccessor for UrlAssetAccessor.

Don't use an SSL certificate

bc37389

This appears to be unnecessary when using the C interface to libcurl, as opposed to cpr. The path to the certificate is still stored, but will go away at a later time.

timoore requested review from weegeekps and lilleyse January 25, 2024 19:25

weegeekps reviewed Jan 25, 2024

View reviewed changes

Add curl options and tracing

c3721fe

Add back the buffersize option from HttpAssetAccessor and also options suggested in the review. Replace vsgCs tracing macro with Cesium Native tracing macro.

Add back use of local SSL certificate

c654554

Test to see if fixes Windows problem. If it does, that is probably a symptom of other configuration issues.

Use curl CURLSSLOPT_NATIVE_CA to use host certificates

e602ddd

Hopefully this will remove the dependency on a local certificate.

timoore mentioned this pull request Jan 30, 2024

Refresh tileset should not re-download tile data #650

Open

timoore added 2 commits January 30, 2024 18:04

Remove MIT license and add Cesium GS copyright

561cb95

Merge branch 'main' into tmp/merge-main

d206fa8

timoore force-pushed the libcurl-accessor branch from 1ec71a8 to d206fa8 Compare January 31, 2024 01:07

timoore added 3 commits January 31, 2024 02:18

Fix some clang-format issues left over from merge

7b6bd7d

Formatting change for clang-format test

5c8509a

lilleyse reviewed Jan 31, 2024

View reviewed changes

Cleanup from review suggestions

83c366d

Also, move CurlHandle out of public UrlAssetAccesor.h

lilleyse added the performance label Feb 5, 2024

timoore mentioned this pull request Feb 5, 2024

Accessor cache using CeslumAsync::SqliteCache #666

Merged

Merge branch 'main' into libcurl-accessor

9775424

Merge branch 'main' into libcurl-accessor

23360a6

lilleyse approved these changes Feb 20, 2024

View reviewed changes

timoore added 2 commits February 20, 2024 16:26

Merge branch 'main' into libcurl-accessor

9020f34

Add copyright and license to UrlAssetAccessor.cpp

36cd4d6

Update ThirdParty.extra.json.

timoore merged commit 7cd8b4b into main Feb 20, 2024
3 checks passed

timoore deleted the libcurl-accessor branch February 20, 2024 16:55

lilleyse mentioned this pull request Feb 23, 2024

ion authentication fails for packaged build #695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset accessor that uses libcurl directly #634

asset accessor that uses libcurl directly #634

timoore commented Jan 25, 2024

weegeekps left a comment

weegeekps Jan 25, 2024

timoore Jan 25, 2024

weegeekps Jan 25, 2024

weegeekps Jan 25, 2024

timoore Jan 25, 2024

weegeekps commented Jan 25, 2024

lilleyse commented Jan 26, 2024

timoore commented Jan 26, 2024

timoore commented Jan 26, 2024 •

edited

Loading

lilleyse commented Jan 26, 2024

timoore commented Jan 26, 2024

weegeekps commented Jan 26, 2024

timoore commented Jan 29, 2024

lilleyse commented Jan 29, 2024

r-veenstra commented Jan 30, 2024 •

edited

Loading

lilleyse Jan 31, 2024

lilleyse Jan 31, 2024

lilleyse Jan 31, 2024

lilleyse Jan 31, 2024

timoore Feb 1, 2024

lilleyse Jan 31, 2024

timoore Feb 1, 2024

lilleyse Jan 31, 2024

lilleyse Jan 31, 2024

timoore Feb 1, 2024

lilleyse Jan 31, 2024

timoore Feb 1, 2024

lilleyse Jan 31, 2024

timoore Feb 1, 2024

lilleyse commented Feb 13, 2024

lilleyse left a comment •

edited

Loading

asset accessor that uses libcurl directly #634

asset accessor that uses libcurl directly #634

Conversation

timoore commented Jan 25, 2024

weegeekps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weegeekps commented Jan 25, 2024

lilleyse commented Jan 26, 2024

timoore commented Jan 26, 2024

timoore commented Jan 26, 2024 • edited Loading

lilleyse commented Jan 26, 2024

timoore commented Jan 26, 2024

weegeekps commented Jan 26, 2024

timoore commented Jan 29, 2024

lilleyse commented Jan 29, 2024

r-veenstra commented Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Feb 13, 2024

lilleyse left a comment • edited Loading

Choose a reason for hiding this comment

timoore commented Jan 26, 2024 •

edited

Loading

r-veenstra commented Jan 30, 2024 •

edited

Loading

lilleyse left a comment •

edited

Loading