Avoid excessive reply buffer copy in WinHTTP #2954

SergeyRyabinin · 2024-05-10T18:24:24Z

Issue #, if available:

Unlike in lib curl,

WinHTTP expects a pointer + size to where Windows is going to write us a reply:

WINHTTPAPI BOOL WinHttpReadData(
  [in]  HINTERNET hRequest,
  [out] LPVOID    lpBuffer,
  [in]  DWORD     dwNumberOfBytesToRead,
  [out] LPDWORD   lpdwNumberOfBytesRead
);

https://learn.microsoft.com/en-us/windows/win32/api/winhttp/nf-winhttp-winhttpreaddata
while C++'s iostream (it the central point of requests/response handling in the SDK from the initial design) manipulates C++'s streams:

// inserts blocks of characters 
basic_ostream& write( const char_type* s, std::streamsize count );

https://en.cppreference.com/w/cpp/io/basic_iostream
and can't provide raw pre-allocated buffer that WinHTTP wants.

Should WinHTTP provide std::iostream interface - this would not be an issue.

Description of changes:

Create a hack-y class to access raw pointers of the underlying buffer of the streambuffer to perform writes directly to the pptr pointer.

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sbiscigl · 2024-05-10T19:14:40Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                    }
+
+                    read = 0;
+                    while (writerFunc(dstBegin, dstSz, read) && read > 0)


instead of having two while loops they seem to have similar bodies, seems like we could use a do while here to avoid dry

there are 1 while loop. While it is a bit ugly to have similar conditions, it looks to have a bit more clear logic.

DmitriyMusatkin · 2024-05-16T16:30:32Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                            }
+                        }
+                    }
+                    return totalRead;


under what conditions are you getting to this line? only way out of while is to return. is this expected or while is missing a break somewhere?

same as dimitry also if the line is "unreachable in theory but we need to return something" we should assert use AWS_UNREACHABLE as a invariant

changed to break + proper return

sbiscigl · 2024-05-22T17:02:45Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                            }
+                        }
+                    }
+                    return totalRead;


same as dimitry also if the line is "unreachable in theory but we need to return something" we should assert use AWS_UNREACHABLE as a invariant

sbiscigl · 2024-05-22T17:03:59Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                        if (ioStream.fail()) {
+                            AWS_LOGSTREAM_ERROR("StreamBufProtectedWriter", "Failed to write 1 byte (eof: "
+                                    << ioStream.eof() << ", bad: " << ioStream.bad() << ")");
+                            return 0;


np: please return false/true instead of int

sbiscigl · 2024-05-22T17:05:45Z

tests/benchmark/benchmark.cpp

@@ -10,6 +10,13 @@
 #include <metric/CloudWatchMetrics.h>

 int main(int argc, char *argv[]) {
+    if (1 == argc ||
+        2 == argc && (std::string(argv[1]) == "-h" || std::string(argv[1]) == "--help" ))


sfod · 2024-05-22T19:40:29Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                    return false;
+                }
+
+                static uint64_t WriteWithHelperBuffer(Aws::IOStream& ioStream, const WriterFunc& writerFunc, uint64_t& read)


nit: the return type should be bool

sfod · 2024-05-22T19:40:39Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                    return false;
+                }
+
+                static uint64_t WriteDirectlyToPtr(StreamBufProtectedWriter* pBuffer, const WriterFunc& writerFunc, uint64_t& read)


nit: the return type should be bool

sfod · 2024-05-22T20:42:59Z

src/aws-cpp-sdk-core/include/aws/core/utils/stream/StreamBufProtectedWriter.h

+                    {
+                        ioStream.write(tmpBuf, read);
+                        if (ioStream.fail()) {
+                            AWS_LOGSTREAM_ERROR("StreamBufProtectedWriter", "Failed to write " << tmpBufSz


nit: shouldn't it be "Failed to write " << read?

sbiscigl reviewed May 10, 2024

View reviewed changes

SergeyRyabinin force-pushed the sr/winHttpReplyPerf branch 2 times, most recently from 410b24d to 814f1a3 Compare May 13, 2024 21:50

SergeyRyabinin changed the title ~~Avoid excessive reply buffer copy in WinHTTP using a hack~~ Avoid excessive reply buffer copy in WinHTTP May 13, 2024

SergeyRyabinin requested a review from DmitriyMusatkin May 13, 2024 21:51

SergeyRyabinin marked this pull request as ready for review May 13, 2024 21:52

SergeyRyabinin force-pushed the sr/winHttpReplyPerf branch 9 times, most recently from 7c3111d to 8c472e1 Compare May 16, 2024 00:04

DmitriyMusatkin reviewed May 16, 2024

View reviewed changes

DmitriyMusatkin approved these changes May 16, 2024

View reviewed changes

SergeyRyabinin force-pushed the sr/winHttpReplyPerf branch 2 times, most recently from 911c421 to 7e5292b Compare May 21, 2024 23:24

sbiscigl approved these changes May 22, 2024

View reviewed changes

SergeyRyabinin force-pushed the sr/winHttpReplyPerf branch from 7e5292b to 2e25d47 Compare May 22, 2024 17:15

Avoid excessive reply buffer copy in WinHTTP

57521e6

SergeyRyabinin force-pushed the sr/winHttpReplyPerf branch from 2e25d47 to 57521e6 Compare May 22, 2024 19:35

sfod approved these changes May 22, 2024

View reviewed changes

SergeyRyabinin merged commit 0ac4224 into main May 22, 2024
3 of 4 checks passed

SergeyRyabinin deleted the sr/winHttpReplyPerf branch May 22, 2024 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid excessive reply buffer copy in WinHTTP #2954

Avoid excessive reply buffer copy in WinHTTP #2954

SergeyRyabinin commented May 10, 2024 •

edited

Loading

sbiscigl May 10, 2024

SergeyRyabinin May 13, 2024

DmitriyMusatkin May 16, 2024

sbiscigl May 22, 2024

SergeyRyabinin May 22, 2024

sbiscigl May 22, 2024

sbiscigl May 22, 2024

sbiscigl May 22, 2024

sfod May 22, 2024

sfod May 22, 2024

sfod May 22, 2024

Avoid excessive reply buffer copy in WinHTTP #2954

Avoid excessive reply buffer copy in WinHTTP #2954

Conversation

SergeyRyabinin commented May 10, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyRyabinin commented May 10, 2024 •

edited

Loading