Bump opendal to 0.58.1 and fix fallout (fixes local GCS cache usage) - #2797
Closed
codspeed-hq[bot] wants to merge 6 commits into
Closed
Bump opendal to 0.58.1 and fix fallout (fixes local GCS cache usage)#2797codspeed-hq[bot] wants to merge 6 commits into
codspeed-hq[bot] wants to merge 6 commits into
Conversation
- Required bump of reqwest to 0.13.0
- Rename and remove features deprecated in that release
- Remove direct `reqsign` dependency that was not used in the code
- Add new (optional) opendal-* dependencies that were split out of opendal
I first did just a partial Cargo.lock update for only the dependencies that were bumped in this PR. But the churn was quite big when just doing that. So I figured why not do a full, clean `cargo update` of the `Cargo.lock` file.
- `HttpClientLayer` was removed and now has to be built from the `OperationContext` - `LoggingLayer` was moved to its own crate - `allow_anonymous` was deprecated in favor of `skip_signature` - Use `sha256` instead of `sha1`, as that is quasi-deprecated. I'm not sure if this is actually necessary. I think it fixed some issue in the tests? In any case, I think this is a good change - Check if `native_certs` are available when building a reqwest client, and if not set the TLS certs to an empty vector. With reqwest 0.13, the client would fail otherwise.
This fixes the Clippy findings after the dep bump and `cargo update`.
![codspeed-hq[bot]](https://github.com/ghassets/images/gravatars/gravatar-user-420.png){kind=avatar}
`CacheWrite::put_bytes` compresses stdout and stderr, which are usually a handful of bytes, but it went through `zstd::stream::copy_encode`, which cannot know the input size up front. zstd therefore allocated and zeroed a compression context sized for the default 2 MiB window on every call, and profiles showed that single memset accounting for up to a third of the cost of writing a cache entry. Compress those blobs with an explicit encoder and pledge the source size, so zstd sizes its workspace for the actual input. Compression ratio is unchanged; writing cache entries gets 16-47% cheaper depending on the benchmark.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
authorized_usercredential apache/opendal#6287The gcs cache can currently not be used with
authorized_userapplication-default credentials, as this was not supported inreqsignversion 0.16.5, whichopendalversion0.55.0depended on. This was fixed in a later version of reqsign and since has been updated inopendalWith this, the gcs cache should Just Work ™️ after a
gcloud auth application-default login.Changes
I split this PR into the following commits for easier commit-by-commit review:
Bump opendal to 0.58.1
reqsigndependency that was not used in the codeRun cargo update
I first did just a partial Cargo.lock update for only the dependencies that were bumped in this PR. But the churn was quite big when just doing that. So I figured why not do a full, clean
cargo updateof theCargo.lockfile.Fix fallout in code
HttpClientLayerwas removed and now has to be built from theOperationContextLoggingLayerwas moved to its own crateallow_anonymouswas deprecated in favor ofskip_signaturesha256instead ofsha1, as that is quasi-deprecated. I'm not sure if this is actually necessary. I think it fixed some issue in the tests? In any case, I think this is a good changenative_certsare available when building a reqwest client, and if not set the TLS certs to an empty vector. With reqwest 0.13, the client would fail otherwise.Size the zstd compression context to the stdout/stderr length
Addresses the benchmark regressions reported by CodSpeed on this PR. The
cargo updateabove upgraded the bundled zstd C library from 1.5.6 to 1.5.7, which is ~7% more expensive per byte compressed and added a new block splitter, and that showed up in the cache-entry benchmarks.Rather than pinning zstd back, this removes a much larger inefficiency that the profiles exposed:
CacheWrite::put_bytesusedzstd::stream::copy_encode, which cannot know the input size, so zstd allocated and zeroed a compression context sized for the default 2 MiB window for every stdout/stderr blob — typically a handful of bytes. That single memset accounted for up to a third of the cost of writing a cache entry. Pledging the (known) source size lets zstd size its workspace for the actual input. Compression ratio is unchanged.