Is passing a context into the S3 API methods necessary for optimizing push? Storing a context in a struct is discouraged as it obfuscates the lifetime of the operation that the context is being applied to. I am not totally against doing so (io.WriteCloser falls within the exception criteria) but I would much prefer to have that discussion in its own PR.

Yes, I am aware of this indeed, and yes it's sucky; the reason why the context is the writer field is that the writer makes a bunch of S3 API calls that require passing the context to them (unless we opt not to use the *WithContext() SDK methods at the expense of not being able to propagate the cancellations from the driver calls).

Would it be feasible to split the context plumbing improvements out to a follow-up PR? I have a couple things to discuss which would otherwise bog down the review of the push optimization work in this PR. (That reminds me, I really need to finish writing that blog post about contexts...)

I dont mind, happy to rip it out; as for the AWS SDK calls that accept this context as a param, do you prefer dropping the *WithContext() calls or temporarily passing context.TODO() to them with some comment?

I'd prefer reverting the calls so they don't show up as changed lines in the PR diff. It's easier to review that way.

Done in 0463ade

Curious about how you wanna go about this. I'm worried you'll suggest some wild context wrappers 🙃

I'm worried you'll suggest some wild context wrappers 🙃

Don't worry, I won't.

This is exactly the kind of thing I'm talking about regarding context lifetimes.

Yes, I commented above. Wish we could somehow link related comments other than like this: #4066 (comment)

Have you considered lazily allocating the pending buffer on demand? That'd cut down on memory consumption when writing files small enough to fit into a single chunk.

☝️ I see this was a "non-blocking" comment, but curious if you gave this some consideration.

I said in one of my comments that it's something I need to have a think about but would move it to the follow-up. It's a sound suggestion worth considering, indeed.

Would it be feasible to split the context plumbing improvements out to a follow-up PR? I have a couple things to discuss which would otherwise bog down the review of the push optimization work in this PR. (That reminds me, I really need to finish writing that blog post about contexts...)

I'm worried you'll suggest some wild context wrappers 🙃

Don't worry, I won't.

If err is not nil (and not a io.EOF), should offset still be returned, or should it return 0, err ?

If so, then perhaps something like this would be slightly more idiomatic;l

if err != nil && err != io.EOF {
    return 0, err
}
return offset, nil

Returning 0 on error would violate the io.ReaderFrom interface contract.

Just a question; is it worth benchmarking if memoizing len(b.data) and cap(b.data) here would be beneficial (i.e., use an intermediate variable to store those as we're calling it multiple times). Maybe not; just curious.

Yeah, I'm not sure it's worth it. Maybe? 🤷‍♂️ len and cap just return an unexported field from the underlying struct so I'd expect negligible gain here 🤔

No; len(b.data) and cap(b.data) are just accessing fields on the slice header which costs the same as any other struct field read. Compiler explorer reveals that the compiler is able to elide bounds-checks for the expression b.data[len(b.data):cap(b.data)] which it cannot do when those values are copied to local variables. Copying the slice itself to a local variable sounds promising as a way to avoid dereferencing pointers inside the loop, but as compiler explorer shows, the generated AMD64 code immediately spills the slice header fields to the stack.

tl;dr introducing intermediate variables will actually deoptimize the function. Compilers are pretty smart these days.

yes, sorry by "unexported fields" I meant "fields" and yeah, header is the right terminology. Thanks

☝️ I see this was a "non-blocking" comment, but curious if you gave this some consideration.

nit: looks like the extra braces are not needed here

if w.driver.MultipartCombineSmallPart && w.pending.Len() > 0 && w.pending.Len() < int(w.driver.ChunkSize) {

they are not needed, no, but I felt like it'd be nice of these were semantically grouped -- it would have been more obvious if I split these on two separate lines; I'm happy to change back I just feel there are too many &&s in a single condition causing cognitive overload 🤷‍♂️

As we're calling this in a for; wondering if it would be beneficial to change n to an int64, and only cast to an int when returning

I considered this, indeed. I stuck with this at the time. There are a few returns here so I struggle to see the win 🤔 I'm willing to be persuaded though

Yeah, I was curious if benchmarking would show a difference; I just did a quick check, and there's no consistent difference, so not worth changing.

I'd have to dig deeper into the whole flow, but is it intentional to return both n and an err here? (The code higher up at line 1418 does a return 0, err on errors.

I think this is an interesting shout. writer is supposed to implement https://pkg.go.dev/io#Writer

It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p)

Now, the code on line 1418 does NOT write to p which is the byte slice passed into Write. I think it's fine to return 0, err there and n, err here. WDYT @corhere

Returning the number of bytes written from p is a requirement of the io.Writer interface contract. Zero bytes have been written from p if control flow reaches line 1418.

Thanks! That's the part I wanted to dig deeper into, because I suspected that information had to be preserved (even in the error case). I guess the interface is just slightly confusing to require both error and value, but it makes sense in context.

I guess reader := bytes.NewReader(p) could be inlined here, but that's really nitpicking.

Yeah, we could probably be more clever in this code but let's keep it as unless you insist 😄

Yeah, I was curious if benchmarking would show a difference; I just did a quick check, and there's no consistent difference, so not worth changing.

Thanks! That's the part I wanted to dig deeper into, because I suspected that information had to be preserved (even in the error case). I guess the interface is just slightly confusing to require both error and value, but it makes sense in context.