Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 #397

waahm7 · 2023-12-20T21:27:06Z

Issue #, if available:
#370

Description of changes:

This PR refactors our GetObject request with Range header logic to be parsed on the client side, as long as the Range includes a start-range.

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

waahm7 · 2023-12-21T23:12:48Z

include/aws/s3/private/s3_auto_ranged_get.h

@@ -53,6 +53,12 @@ struct aws_s3_auto_ranged_get {
    } synced_data;

    uint32_t initial_message_has_range_header : 1;
+
+    bool initial_message_has_start_range;


Using bool because we can not have a pointer to bit field https://stackoverflow.com/questions/13547352/c-cannot-take-address-of-bit-field.

By putting these new variables between two other bit-fields, it prevents those bit-fields from getting packed together, and the only point of a bit-field is to get packed together and reduce the overall size of a struct.

IMHO, the bit-fields in aws-c-s3 aren't worth having because they make the code uglier (need to type != 0 instead of just treating them like a normal bool), and these structs are not a big contributor to memory usage. But if we're going to have bit-fields, we should get some benefit from them. Having bitfields that make your code ugly AND getting no benefit from them... is dumb.

I see how you just wanted to put related variables together though. Consider doing 1 of these:

keep all bit-fields together at the end of the struct (the traditional place to put them)

stop using bit-fields altogether in this struct (find/replace ugly code that does != 0)

make this a bit-field (together with other bit-fields), but yeah you can't pass it by pointer so you'll need to make a temp variable bool initial_message_has_start_range_tmp and pass THAT to your function by pointer, and then set the bit-field to the tmp variable

Thanks for the detailed description, I have updated it to option 1 for now. I would have preferred option 2 but don't want to include that refactor in this PR.

codecov-commenter · 2023-12-21T23:47:37Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (cfc395b) 88.79% compared to head (5ef1b80) 88.82%.

Additional details and impacted files

@@                   Coverage Diff                   @@
##           refactor-empty-file     #397      +/-   ##
=======================================================
+ Coverage                88.79%   88.82%   +0.03%     
=======================================================
  Files                       21       21              
  Lines                     6078     6141      +63     
=======================================================
+ Hits                      5397     5455      +58     
- Misses                     681      686       +5

Files	Coverage Δ
source/s3_auto_ranged_get.c	`97.76% <100.00%> (+0.16%)`	⬆️
source/s3_meta_request.c	`90.80% <100.00%> (-0.01%)`	⬇️
source/s3_util.c	`98.01% <87.17%> (-1.61%)`	⬇️

waahm7 · 2023-12-22T22:13:41Z

source/s3_util.c

+        }
+
+        /* verify that start-range <= end-range */
+        if (*out_initial_message_has_end_range && *out_initial_start_range > *out_initial_end_range) {


When we perform a HeadRequest with startRange > endRange such as bytes=20-10, S3 does not add a Content-Range header in the HeadRequest response, and Content-Length reflects the total length of the object. The CLI downloads the entire object. The current CRT behavior is that it fails with AWS_ERROR_S3_MISSING_CONTENT_RANGE_HEADER at https://github.com/awslabs/aws-c-s3/blob/main/source/s3_auto_ranged_get.c#L510C24-L510C66. According to the HTTP spec, “An int-range is invalid if the last-pos value is present and less than the first-pos.” . This changes it to AWS_ERROR_S3_INVALID_RANGE_HEADER error at request time.

graebm

many trivial comments

graebm · 2023-12-26T18:02:13Z

include/aws/s3/private/s3_auto_ranged_get.h

@@ -53,6 +53,12 @@ struct aws_s3_auto_ranged_get {
    } synced_data;

    uint32_t initial_message_has_range_header : 1;
+
+    bool initial_message_has_start_range;


By putting these new variables between two other bit-fields, it prevents those bit-fields from getting packed together, and the only point of a bit-field is to get packed together and reduce the overall size of a struct.

IMHO, the bit-fields in aws-c-s3 aren't worth having because they make the code uglier (need to type != 0 instead of just treating them like a normal bool), and these structs are not a big contributor to memory usage. But if we're going to have bit-fields, we should get some benefit from them. Having bitfields that make your code ugly AND getting no benefit from them... is dumb.

I see how you just wanted to put related variables together though. Consider doing 1 of these:

keep all bit-fields together at the end of the struct (the traditional place to put them)

stop using bit-fields altogether in this struct (find/replace ugly code that does != 0)

make this a bit-field (together with other bit-fields), but yeah you can't pass it by pointer so you'll need to make a temp variable bool initial_message_has_start_range_tmp and pass THAT to your function by pointer, and then set the bit-field to the tmp variable

source/s3_auto_ranged_get.c

graebm · 2023-12-26T18:43:51Z

source/s3_auto_ranged_get.c

+                        // TODO: Should we align the first part? I need to run some benchmarks before re-adding first
+                        // part alignment.


another benefit of aligning the first part is: it makes it more likely that we get checksums, due to aligning with UploadPart boundaries

but oof, yeah, it would suck to send 2 requests when we could have gotten away with 1

just throwing out thoughts, you've got more of this in your head right now than I do...

We will never get the checksum of the first partial part request, even if we align it to part_size, so we can't validate the checksum. I observed no difference in performance between aligning the first request or not aligning it. Therefore, I have removed the logic for aligning the first part.

graebm · 2023-12-26T19:07:30Z

source/s3_auto_ranged_get.c

+                object_range_end = object_size - 1;
+                if (auto_ranged_get->initial_message_has_end_range) {
+                    object_range_end = aws_min_u64(object_range_end, auto_ranged_get->initial_object_range_end);
+                }


utterly trivial: reads better as if/else

Suggested change

object_range_end = object_size - 1;

if (auto_ranged_get->initial_message_has_end_range) {

object_range_end = aws_min_u64(object_range_end, auto_ranged_get->initial_object_range_end);

}

if (auto_ranged_get->initial_message_has_end_range) {

object_range_end = aws_min_u64(object_range_end, auto_ranged_get->initial_object_range_end);

} else {

object_range_end = object_size - 1;

}

The updated object_range_end is used in aws_min_u64. I have changed it to an if/else structure and updated that line to directly use object_size - 1.

source/s3_util.c

tests/s3_util_tests.c

source/s3_util.c

graebm · 2023-12-26T19:47:55Z

tests/s3_util_tests.c

+        bool initial_message_has_start_range;
+        bool initial_message_has_end_range;
+        uint64_t initial_object_range_start;
+        uint64_t initial_object_range_end;


why are the bools "initial_message" but the ints "initial_object"? Is there some subtle difference I'm not picking up on?

(personally, I would omit the "initial_X" prefix from all of them)

No difference; I just kept it similar to existing initial_message_has_range_header, object_range_start, and object_range_end. I agree that it is confusing, especially initial_object_*. I have updated it to the following:

bool initial_message_has_start_range; bool initial_message_has_end_range; uint64_t initial_range_start; uint64_t initial_range_end;

I think range_start and range_end would be confusing alongside object_range_start and object_range_end, so I have kept the initial prefix since initial_range_end and object_range_end might differ. Plus, object_range_end is part of the synced block, which we don't need for initial_range_end.

…error

Co-authored-by: Michael Graeb <graebm@amazon.com>

# Conflicts: # source/s3_auto_ranged_get.c

waahm7 · 2023-12-27T18:10:04Z

source/s3_meta_request.c

@@ -1355,7 +1357,7 @@ static int s_s3_meta_request_incoming_body(
    }

    if (request->send_data.response_body.capacity == 0) {
-        if (request->has_part_size_response_body) {
+        if (request->has_part_size_response_body && successful_response) {


Ignore the buffer ticket, as the error response might not fit into the reserved buffer.

waahm7 added 10 commits December 20, 2023 10:56

Parse the user provided range header

6b052a1

Add logic to skip head request given start range

43bc2a4

remove allocator unusued

5521490

remove allocator

3854912

Force headObject by missing start range

49d8d51

convert to size_t

273d2d8

fix empty file logic

a001a3f

Fix mock test

0ad2a44

Merge branch 'refactor-empty-file' into refactor-ranged-get-object

edf6b9b

Update comment

745c3bb

waahm7 commented Dec 21, 2023

View reviewed changes

waahm7 added 3 commits December 21, 2023 15:19

Update comments

a65473c

lint fix

6849e21

update comment

824fe27

waahm7 added 6 commits December 21, 2023 15:50

Fix todos

bf0571c

Add the startRange>endRange case todos

118cadc

Refactor the parsing logic

99903d5

remove todo

2e244c2

Fix test

8c8dfa9

Refactor object_size vs content_length

cda3be3

waahm7 commented Dec 22, 2023

View reviewed changes

update comments

b4ad24f

waahm7 marked this pull request as ready for review December 22, 2023 22:19

waahm7 changed the title ~~WIP | Refactor GetObject request with Range Header to Skip HeadRequest - Part 3~~ Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 Dec 22, 2023

waahm7 added 5 commits December 22, 2023 15:25

Add parse range header tests

939c395

First part size alignment

8a2b2ab

Remove alignment, as it doesn't appear to affect the benchmarks.

9be9cb0

Clean up

50b4f22

Make sure CI is working even with alignment

68521d9

Fix the object range to remain 'not satisfiable' even with alignment.

d7ca4b4

graebm reviewed Dec 26, 2023

View reviewed changes

waahm7 and others added 18 commits December 26, 2023 11:52

Fix bug where response body can be greator than part_size in case of …

ff2ca61

…error

remove invalid range

7270195

remove alignment and fix etag in case of first_part_size_mismatch

2f1e084

Update source/s3_auto_ranged_get.c

4edd1c1

Co-authored-by: Michael Graeb <graebm@amazon.com>

Update source/s3_auto_ranged_get.c

1e77d70

Co-authored-by: Michael Graeb <graebm@amazon.com>

Update comment

424c939

refactor parsing function

49c38e4

Fix in test as well

e60efc3

remove unused variable

28db482

PR Feedback

254a152

Add test for empty file with range case

9ca78cf

Rename variables

0c0cc98

Update comments

b077d39

Fix another bug

093945c

Merge branch 'refactor-empty-file' into refactor-ranged-get-object

f2dfa19

# Conflicts: # source/s3_auto_ranged_get.c

remove space

0c101b7

comment cleanup

7b703a0

Remove bytes=-0 test

aababa8

waahm7 commented Dec 27, 2023

View reviewed changes

Update log

5ef1b80

graebm approved these changes Dec 27, 2023

View reviewed changes

waahm7 merged commit 879a01e into refactor-empty-file Dec 28, 2023
30 checks passed

waahm7 deleted the refactor-ranged-get-object branch December 28, 2023 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 #397

Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 #397

waahm7 commented Dec 20, 2023 •

edited

Loading

waahm7 Dec 21, 2023

graebm Dec 26, 2023

waahm7 Dec 27, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

waahm7 Dec 22, 2023

graebm left a comment

graebm Dec 26, 2023

graebm Dec 26, 2023

waahm7 Dec 26, 2023

graebm Dec 26, 2023

waahm7 Dec 26, 2023

graebm Dec 26, 2023

waahm7 Dec 27, 2023

waahm7 Dec 27, 2023 •

edited

Loading

		// TODO: Should we align the first part? I need to run some benchmarks before re-adding first
		// part alignment.

Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 #397

Refactor GetObject request with Range Header to Skip HeadRequest - Part 3 #397

Conversation

waahm7 commented Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

graebm 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahm7 Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

waahm7 commented Dec 20, 2023 •

edited

Loading

waahm7 Dec 27, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

waahm7 Dec 27, 2023 •

edited

Loading