Repository navigation
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
454fdc6 to
1cc552d
Compare
1cc552d to
a2f09f3
Compare
|
a2f09f3 to
c60ccf8
Compare
c60ccf8 to
b2177c0
Compare
|
Want your agent to iterate on Greptile's feedback? Start a greploop in Claude Code and it will work through the open comments and keep going until this PR reviews clean. |
b2177c0 to
148e6dd
Compare
adriencaccia
left a comment
There was a problem hiding this comment.
Seen together, let's remove the single part upload path and always use multipart, albeit with a single part for archives under 64MiB.
148e6dd to
810a9e5
Compare
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
olgtm: the metadata snapshot update should have its morrir conterpart updated in platform repo in order to make sure metadata stays compatible, hit me up for more info.
Also some tests could be trimmed, the /deslop skill that we share in platform repo is quite a good judge for it usually
S3 rejects single uploads above 5 GiB, and a single connection to S3 only reaches about 20-25 MiB/s on GitHub-hosted runners, so large profile archives were slow or impossible to upload. Every archive is now sent as an S3 multipart upload, replacing the single-request upload: about two parts per concurrent upload, each between 16 MiB and 256 MiB, so a small archive is a single part. The md5 of every part is computed in the same pass as the archive md5 and sent in the upload metadata (version 12) as `profileMultipart`. The API answers with `multipartUploadUrls`: the parts are uploaded 8 at a time (overridable with `CODSPEED_UPLOAD_CONCURRENCY`), each with its own retries, then the upload is completed with the part ETags in order. This applies to both on-disk and in-memory (gzip) archives. This requires an upload endpoint that accepts metadata version 12 and answers with `multipartUploadUrls`. Walltime profile folders above 5 GiB are no longer gzipped on disk to fit in a single request, and the runner no longer caps the archive size itself: the upload endpoint rejects archives above its limit, with the reason shown in the runner output, so the limit can change without a runner release. Archives are now hashed while streaming on the blocking thread pool, instead of being read whole into memory on the async runtime. Closes COD-3700 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replace the md5 of each part and of the archive with CRC64NVME checksums, and describe the profile archive with a single `profileArchive` field in the upload metadata (version 12): its encoding, size and CRC64NVME, and the size and CRC64NVME of its parts. It replaces `profileEncoding`, `profileMd5` and `profileMultipart`. Each part is hashed once, and the part CRCs are combined into the archive's. S3 can check a CRC64NVME on the whole archive of a multipart upload, which md5 does not support, so a completion that leaves out a part or assembles another archive is rejected, and not only a corrupted part. It is also much cheaper to compute: about 0.5s for a 6 GiB archive on a GitHub-hosted runner, against 24s for the part and archive md5s. The upload endpoint now answers with `multipartUpload`, replacing `multipartUploadUrls`: a presigned request per part and one to complete the upload, each with the headers to send as is, as they are part of the signature. The runner sends them without knowing which ones S3 checks, so the endpoint can change them without a runner release. `crc-fast` is pinned to 1.9, the last release supporting Rust 1.88, which requires `crc` 3.3. Refs COD-3700 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
810a9e5 to
6363b74
Compare
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
OLGTM!
I guess that CI failure is because prod does not yeat accept the new upload metadata. Let's keep this in mind before merging/releasing this and make sure everything's been deployed
2c2bbfc to
acfbe6a
Compare
Upload profile archives as S3 multipart uploads, with parts sent concurrently and checked against CRC64NVME checksums.
S3 rejects a single upload request above 5 GiB, so large walltime and memory profile archives could not be uploaded (walltime folders above 5 GiB were gzipped on disk to try to fit). Even below that limit, a single connection to S3 only reaches about 20-25 MiB/s on GitHub-hosted runners, so a 1 GiB archive took close to a minute to upload.
How it works
profileMd5andprofileEncodingbecomeprofileArchiveMetadata(encoding,size,crc64nvme,partSize,partCrc64nvmes).multipartUploadinstead ofuploadUrl: one presignedUploadPartrequest per part (parts) and a presignedCompleteMultipartUploadrequest (complete), each as{ url, headers }. The headers are part of the signature and are sent as is, so S3 checks each part and the whole object against the checksums. Parts are uploaded 8 at a time, each with its own retries, then the ETags are sent in part order to complete the upload.200status, so the response body is checked for an error. Errors S3 documents as transient (InternalError,ServiceUnavailable,SlowDown,RequestTimeout) and dropped connections are retried; others such asInvalidPartfail right away.upload::s3module.CODSPEED_UPLOAD_CONCURRENCYoverrides the number of concurrent part uploads.Other changes
md5dependency is replaced bycrc-fast.Measurements
Concurrency sweep on a 6 GiB walltime archive (25 parts of 256 MiB), which led to the default of 8:
ubuntu-latestSmaller archives with the final part layout, concurrency 1 (close to the previous single request) vs 8:
ubuntu-latestubuntu-latestHashing a 6 GiB archive, comparing the md5 the runner computed so far with the alternatives. The CRC64NVME of the whole archive is combined from the part CRCs, so each byte is hashed once and the whole + parts pass costs the same as hashing the whole archive alone:
ubuntu-latest(EPYC 7763, 2 cores)The backend support for the version 12 upload metadata is not released yet, so the upload cannot be verified end to end against production for now.
Closes COD-3700