Dynamic default part size #575

TingDaoK · 2025-10-03T00:05:29Z

Issue #, if available:

When the client is configured to allow using more memory, it makes more sense to default to larger range instead of a static 8MiB range. Based on the object characteristics stored in S3, memory constraints, and throughput target, we now dynamic calculate the range for getting the object.
Parse the etag response from s3, as it will be <hash>-<number of part>, so that we can have an estimated part size that S3 stores the object.
Keep the discovery phase to use the 8MiB as a default fallback.

Description of changes:

Fix a race condition in the cancel tests and introduced a compiling flag to allow the test to stub into the function for testing the specific case.

TODO:

Buffer Pool Refactor
- Currently the default buffer pool implementation will not handle the large range allocation from the pool.
- To avoid wasting too many memory for small objects, we cannot just increase the buffer pool chunk size
- We will refactor the buffer pool to handle different size chunks as a quick follow up.
- Buffer pool refactor #584
Integrate
- Integrate the buffer pool refactor and the dynamic size change
- Apply reasonable upper bound and alignment for the implementation
- Tests
S3 Express Optimization
- Add a reasonable upbound for the range to fetch, maybe 1GiB for S3 GP, and 128MiB for S3Express
- Refactor the math to derive the number of connections for s3 express. referring to Improve Scheduling Algorithm for Different BucketTypes and TransferDirection aws-s3-transfer-manager-rs#105
Fuzz tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2025-10-03T17:39:22Z

Codecov Report

❌ Patch coverage is 89.94975% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.12%. Comparing base (6b38088) to head (5d7d268).

Files with missing lines	Patch %	Lines
source/s3_util.c	91.20%	8 Missing ⚠️
source/s3_auto_ranged_get.c	90.00%	3 Missing ⚠️
source/s3_client.c	85.71%	3 Missing ⚠️
source/s3_auto_ranged_put.c	91.30%	2 Missing ⚠️
source/s3_default_meta_request.c	0.00%	2 Missing ⚠️
source/s3_meta_request.c	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #575      +/-   ##
==========================================
+ Coverage   88.97%   89.12%   +0.14%     
==========================================
  Files          23       23              
  Lines        7201     7336     +135     
==========================================
+ Hits         6407     6538     +131     
- Misses        794      798       +4

Files with missing lines	Coverage Δ
source/s3_request.c	`87.16% <100.00%> (+3.94%)`	⬆️
source/s3_auto_ranged_put.c	`91.98% <91.30%> (-0.25%)`	⬇️
source/s3_default_meta_request.c	`94.25% <0.00%> (-1.13%)`	⬇️
source/s3_meta_request.c	`91.04% <86.66%> (-0.09%)`	⬇️
source/s3_auto_ranged_get.c	`97.31% <90.00%> (-0.69%)`	⬇️
source/s3_client.c	`91.61% <85.71%> (-0.09%)`	⬇️
source/s3_util.c	`95.58% <91.20%> (-1.57%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TingDaoK · 2025-10-06T22:29:41Z

CMakeLists.txt


 include(CTest)
 if (BUILD_TESTING)
+    add_definitions(-DAWS_C_S3_ENABLE_TEST_STUBS)


any concern adding this def here? Or it's better to keep as a separate one.

pro: Nothing will break.
con: Maybe people built with tests in prod and don't want to have this stub enabled.

do we enable this ourselves when we package java, python, etc...?
probably not a big concern in practice, it add a private api. we already expose a bunch of private apis used by tests only

https://github.com/awslabs/aws-crt-python/blob/main/crt/CMakeLists.txt#L26
https://github.com/awslabs/aws-crt-java/blob/main/pom.xml#L185
Yeah, we do set this to off when we build the bindings.
Also, we should not build the C tests when we package for the CRT bindings.

source/s3_auto_ranged_get.c

…ata delivery in the sequential order

DmitriyMusatkin · 2025-10-21T23:49:26Z

include/aws/s3/private/s3_request.h

+        bool memory_allocated_from_pool;
    } crt_info_metrics;

+    struct {


thinking out loud:
maybe we should unify this with

aws-c-s3/include/aws/s3/private/s3_meta_request_impl.h

Line 297 in 332dd22

struct aws_s3_mpu_part_info {

.
we can probably do some of the same sanity checks in upload path as well

include/aws/s3/private/s3_util.h

include/aws/s3/s3_client.h

DmitriyMusatkin · 2025-10-28T21:27:50Z

source/s3_auto_ranged_get.c


-            if (aws_http_headers_get(request->send_data.response_headers, g_etag_header_name, &etag_header_value)) {
+                if (!auto_ranged_get->initial_message_has_if_match_header) {
+                    /* Store ETag if needed for If-Match header */


why only if needed? isnt it easier to just always store it (i.e. the old way)

the old way also only store the etag when initial_message_has_if_match_header is false.

source/s3_auto_ranged_get.c

DmitriyMusatkin · 2025-10-28T22:44:44Z

source/s3_util.c

-    AWS_PRECONDITION(response_headers);

-    struct aws_byte_cursor content_range_header_value;
+    /* Expected Format of header is: "bytes StartByte-EndByte/TotalObjectSize" */


does s3 always respect that or there are cases where it skips start and end (which is valid in http spec)

https://httpwg.org/specs/rfc9110.html#field.content-range
I don't think the Spec supports skipping those for a success response. Not like Range to be sent for the request. The Content-range in the response will need to have both start and end for a range-resp

DmitriyMusatkin · 2025-10-28T22:50:56Z

source/s3_util.c

+
+int aws_s3_extract_parts_from_etag(struct aws_byte_cursor etag_header_value, uint32_t *out_num_parts) {
+
+    AWS_PRECONDITION(out_num_parts);


lets do AWS_ERROR_PRECONDITION here and in other places

DmitriyMusatkin · 2025-10-28T22:56:28Z

source/s3_util.c

+    while (aws_byte_cursor_next_split(&remaining_cursor, '-', &substr)) {
+        split_count++;
+        if (split_count == 2) {
+            /* The ETag should follow the pattern <hash>-<parts_count>, so the second part is the parts count. */


while its should be the case in most cases, the spec we have from s3 for etag is -.
value in theory can contain dashes. we should find - from the back and try to parse from that until the end as a number

We need to confirm this with S3, if the value contains -, find it from the back will still result in unexpected behaviors.

As far as I find, the etag value should be a hash value? So it should not contain -

I'd rather to force the etag to be exactly have one or less - in it. So that if it's not the expected format, we will error out and fallback, instead of reading from the back try to parse the case where it's not formatted as expect.

Co-authored-by: Dmitriy Musatkin <[email protected]>

TingDaoK added 6 commits October 1, 2025 17:02

WIP

d804f65

add extract the number of parts from etag

5fb1c44

fix tests

59c18b2

fix build

f0ee675

apply the dynamic size

772c4a0

fix the logic for empty file

a12d2cb

TingDaoK added 5 commits October 3, 2025 11:19

fix compiler warning

3796c1e

add metrics

350281d

get part number as well

cbcc4e5

try to fix the flaky test

3eadf8a

let's add a flag and ifdef

2141cfc

TingDaoK commented Oct 6, 2025

View reviewed changes

TingDaoK added 4 commits October 7, 2025 12:03

adding test

d196a83

more tests

2982e3e

fix some errors

a58305c

compilers

66424cc

TingDaoK marked this pull request as ready for review October 7, 2025 23:04

TingDaoK added 8 commits October 7, 2025 16:08

more warnings

1686881

remove my local config

1a334d0

increase the memory limit so that we are not limited by that

2e900b4

in case of retry, let's record the succeed only metrics as well

e0fe6d8

Merge branch 'main' into dynamic-default-part-size

c7ebc80

fix the mem metrics logic

bb61f3b

well, on 32bit it will default to 1

2a4f6bb

remove the hard coded 8MiB

72e6976

TingDaoK commented Oct 15, 2025

View reviewed changes

source/s3_auto_ranged_get.c Outdated Show resolved Hide resolved

TingDaoK added 3 commits October 20, 2025 12:06

add checks for the response to match the request and makes sure the d…

e6a395b

…ata delivery in the sequential order

let's see what breaks

d1fe086

fix the curosr parsing logic

587c3f9

TingDaoK added 3 commits October 20, 2025 15:53

fix couple corner case

982fcce

another concern case

c0de491

well. The mock server was wrong the whole time...

64a6e5a

DmitriyMusatkin reviewed Oct 21, 2025

View reviewed changes

DmitriyMusatkin approved these changes Oct 28, 2025

View reviewed changes

TingDaoK and others added 5 commits October 29, 2025 14:04

merge

f0199da

fix

af29d4f

Apply suggestion from @DmitriyMusatkin

287b507

Co-authored-by: Dmitriy Musatkin <[email protected]>

address comments

54d5c38

collect part range for auto-range-put as well (#588)

5d7d268


		int aws_s3_extract_parts_from_etag(struct aws_byte_cursor etag_header_value, uint32_t *out_num_parts) {

		AWS_PRECONDITION(out_num_parts);

Dynamic default part size #575

Are you sure you want to change the base?

Dynamic default part size #575

Uh oh!

Conversation

TingDaoK commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TingDaoK Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TingDaoK commented Oct 3, 2025 •

edited

Loading

codecov-commenter commented Oct 3, 2025 •

edited

Loading

TingDaoK Oct 22, 2025 •

edited

Loading