Repository navigation
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
454fdc6 to
1cc552d
Compare
lvaroqui
changed the title
1cc552d to
a2f09f3
Compare
lvaroqui
changed the title
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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
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
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
| pub size: u64, | ||
| pub part_size: u64, |
There was a problem hiding this comment.
potential nitpick: wouldn't usize here make more sense?
| pub(super) fn concurrent_part_uploads() -> usize { | ||
| *CONCURRENT_PART_UPLOADS | ||
| } |
There was a problem hiding this comment.
Nitpick: do we really need a function layer of abstraction to hide the lazylock? Reading the code I find it slightly clearer to use *CONCURRENT_PART_UPLOAD directly usually. Debatable
| /// More parts than concurrent uploads, so that the upload slots freed by fast parts | ||
| /// pick up remaining work instead of idling while the slowest part finishes |
There was a problem hiding this comment.
Extremely nitpicky: The More part per concurrent upload is slightly misleading and does not add much value since we are defining a number of part per concurrent upload that is a >1 integer lol
The fact that we assign a number of parts per concurrent upload to have fast parts' upload worker be picked up by others can be interesting though
There was a problem hiding this comment.
Much less nitpicky: It should state that it's a target though, and it's quite hard to guess what happens when
PARTS_PER_CONCURRENT_UPLOAD * CONCURRENT_PART_UPLOADS_ENV < archive_size / MAX_PART_SIZE
Is this an error case? Do we simply have more than the target?
| pub async fn new_compressed_in_memory(data: Vec<u8>) -> Result<Self> { | ||
| Self::new(ProfileArchiveContent::CompressedInMemory { data: data.into() }).await | ||
| } |
There was a problem hiding this comment.
For my curiosity, does this becoming async change anything? Cause in terms of type system there's not much change is there?
The fact is the operation performed in this function is still sync, putting async in there does not really change this does it?
| upload_multipart_profile_archive( | ||
| &upload_data.multipart_upload_urls, | ||
| &profile_archive.multipart, | ||
| &profile_archive.content, | ||
| ) | ||
| .await |
There was a problem hiding this comment.
Do we really need this function? We could do everything in the body here now that we do not manage single part uplaod
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
3 participants
Upload profile archives of 64 MiB and more as S3 multipart uploads, with parts sent concurrently.
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
profileMultipart(size,partSize,partMd5s).multipartUploadUrls(partUrls,completeUrl) instead ofuploadUrl. Each part URL is presigned with the part'sContent-MD5, so S3 checks every part. Parts are uploaded 8 at a time, each with its own retries, then the ETags are sent in part order to the completion URL. S3 can report a failed completion with a 200 status, so the response body is also checked for an error.upload::s3module.CODSPEED_UPLOAD_CONCURRENCYoverrides the number of concurrent part uploads.Archives below 64 MiB keep the single upload request, and their metadata is unchanged apart from the version.
Other changes
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-latestThe backend support for
profileMultipartis not released yet, so the multipart path cannot be verified end to end against production for now.Closes COD-3700