http: set TCP_NODELAY when creating HTTP server #30675

pull romanz wants to merge 1 commits into bitcoin:master from romanz:http-nodelay changing 1 files +6 −0
  1. romanz commented at 3:22 pm on August 19, 2024: contributor

    Otherwise, the default HTTP server config may result in high latency, due to Nagle’s algorithm (on the server) and delayed ACK (on the client):

    [1] https://www.extrahop.com/blog/tcp-nodelay-nagle-quickack-best-practices [2] https://eklitzke.org/the-caveats-of-tcp-nodelay

    Without the fix, fetching a small block takes ~40ms (when connection keep-alive is enabled):

     0$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin 
     1
     2Server Software:        
     3Server Hostname:        localhost
     4Server Port:            8332
     5
     6Document Path:          /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
     7Document Length:        25086 bytes
     8
     9Concurrency Level:      1
    10Time taken for tests:   4.075 seconds
    11Complete requests:      100
    12Failed requests:        0
    13Keep-Alive requests:    100
    14Total transferred:      2519200 bytes
    15HTML transferred:       2508600 bytes
    16Requests per second:    24.54 [#/sec] (mean)
    17Time per request:       40.747 [ms] (mean)
    18Time per request:       40.747 [ms] (mean, across all concurrent requests)
    19Transfer rate:          603.76 [Kbytes/sec] received
    20
    21Connection Times (ms)
    22              min  mean[+/-sd] median   max
    23Connect:        0    0   0.0      0       0
    24Processing:     0   41   4.1     41      42
    25Waiting:        0    0   0.1      0       1
    26Total:          0   41   4.1     41      42
    27
    28Percentage of the requests served within a certain time (ms)
    29  50%     41
    30  66%     41
    31  75%     41
    32  80%     41
    33  90%     42
    34  95%     42
    35  98%     42
    36  99%     42
    37 100%     42 (longest request)
    

    With the fix, it takes ~0.2ms:

     0$ ab -k -c 1 -n 1000 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin 
     1
     2Benchmarking localhost (be patient)
     3Completed 100 requests
     4Completed 200 requests
     5Completed 300 requests
     6Completed 400 requests
     7Completed 500 requests
     8Completed 600 requests
     9Completed 700 requests
    10Completed 800 requests
    11Completed 900 requests
    12Completed 1000 requests
    13Finished 1000 requests
    14
    15
    16Server Software:        
    17Server Hostname:        localhost
    18Server Port:            8332
    19
    20Document Path:          /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
    21Document Length:        25086 bytes
    22
    23Concurrency Level:      1
    24Time taken for tests:   0.194 seconds
    25Complete requests:      1000
    26Failed requests:        0
    27Keep-Alive requests:    1000
    28Total transferred:      25192000 bytes
    29HTML transferred:       25086000 bytes
    30Requests per second:    5147.05 [#/sec] (mean)
    31Time per request:       0.194 [ms] (mean)
    32Time per request:       0.194 [ms] (mean, across all concurrent requests)
    33Transfer rate:          126625.50 [Kbytes/sec] received
    34
    35Connection Times (ms)
    36              min  mean[+/-sd] median   max
    37Connect:        0    0   0.0      0       0
    38Processing:     0    0   0.0      0       0
    39Waiting:        0    0   0.0      0       0
    40Total:          0    0   0.0      0       0
    41
    42Percentage of the requests served within a certain time (ms)
    43  50%      0
    44  66%      0
    45  75%      0
    46  80%      0
    47  90%      0
    48  95%      0
    49  98%      0
    50  99%      0
    51 100%      0 (longest request)
    
  2. DrahtBot commented at 3:22 pm on August 19, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theStack, tdb3, achow101
    Concept ACK brunoerg, sipa
    Stale ACK pinheadmz

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label RPC/REST/ZMQ on Aug 19, 2024
  4. in src/httpserver.cpp:395 in f983496d78 outdated
    387@@ -388,6 +388,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
    388             if (i->first.empty() || (addr.has_value() && addr->IsBindAny())) {
    389                 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
    390             }
    391+            // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
    392+            evutil_socket_t fd = evhttp_bound_socket_get_fd(bind_handle);
    393+            int nOne = 1;
    394+            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
    395+                LogPrintf("WARNING: Unable to set TCP_NODELAY on RPC server socket, continuing anyway\n");
    


    andrewtoth commented at 3:29 pm on August 19, 2024:
    LogPrintf is deprecated. See https://github.com/bitcoin/bitcoin/pull/29641

    romanz commented at 4:30 pm on August 19, 2024:
    Many thanks! Fixed in f14484184f8cae134f65801647280c2c1f4930b2.
  5. romanz commented at 3:31 pm on August 19, 2024: contributor
    TCP_NODELAY has been set on p2p connections by #6867.
  6. romanz force-pushed on Aug 19, 2024
  7. pinheadmz commented at 4:16 pm on August 19, 2024: member
    concept ACK, I’m kinda surprised we don’t have this already. I did a quick check into libevent and don’t see any helpers there like evutil_make_listen_socket_reuseable() (which sets SO_REUSEADDR) so this might be the best approach
  8. theStack commented at 5:21 pm on August 19, 2024: contributor
    Concept ACK
  9. brunoerg commented at 5:36 pm on August 19, 2024: contributor
    Concept ACK
  10. sipa commented at 2:25 am on August 20, 2024: member
    Concept ACK
  11. in src/httpserver.cpp:393 in f14484184f outdated
    387@@ -388,6 +388,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
    388             if (i->first.empty() || (addr.has_value() && addr->IsBindAny())) {
    389                 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
    390             }
    391+            // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
    392+            evutil_socket_t fd = evhttp_bound_socket_get_fd(bind_handle);
    393+            int nOne = 1;
    


    tdb3 commented at 2:41 am on August 20, 2024:

    nit: could use n_one to better align with style guidelines.

    https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

    Variable (including function arguments) and namespace names are all lowercase and may use _ to separate words (snake_case).


    sipa commented at 2:46 am on August 20, 2024:
    Following the style guide it’d just be called one. The n prefix is a Hungarian notation prefix for “number”, which we no longer use.

    romanz commented at 5:03 am on August 20, 2024:
    Thanks! Fixed to one in c1bec82697.

    tdb3 commented at 9:21 am on August 20, 2024:
    Ah, that’s right (no hungarian). Thanks sipa
  12. tdb3 commented at 2:44 am on August 20, 2024: contributor
    Concept ACK Good find! Left just a nit for now, but plan to return to review in more detail (and possibly run some tests for different size transfers/blocks)
  13. romanz force-pushed on Aug 20, 2024
  14. in src/httpserver.cpp:394 in c1bec82697 outdated
    387@@ -388,6 +388,12 @@ static bool HTTPBindAddresses(struct evhttp* http)
    388             if (i->first.empty() || (addr.has_value() && addr->IsBindAny())) {
    389                 LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
    390             }
    391+            // Set the no-delay option (disable Nagle's algorithm) on the TCP socket.
    392+            evutil_socket_t fd = evhttp_bound_socket_get_fd(bind_handle);
    393+            int one = 1;
    394+            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&one, sizeof(int)) == SOCKET_ERROR) {
    


    theStack commented at 10:52 pm on August 24, 2024:

    nit: imho slightly preferred, for not having to repeat the type

    0            if (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (sockopt_arg_type)&one, sizeof(one)) == SOCKET_ERROR) {
    

    romanz commented at 6:25 am on August 25, 2024:
    Thanks! Fixed in 03d49d0f25.
  15. theStack approved
  16. theStack commented at 11:04 pm on August 24, 2024: contributor

    Tested ACK c1bec8269712059705d5f416257ad5201ec3d4a8

    With the benchmark instructions from the opening post (using Apache’s ab tool, found in the apache2-utils package on Debian Linux) adapted to fetch a recent block, this leads to a >10x speed-up on my machine: mean Time per request is 46.617ms on master vs. 3.717ms on the PR.

  17. DrahtBot requested review from tdb3 on Aug 24, 2024
  18. DrahtBot requested review from brunoerg on Aug 24, 2024
  19. DrahtBot requested review from pinheadmz on Aug 24, 2024
  20. DrahtBot requested review from sipa on Aug 24, 2024
  21. http: set TCP_NODELAY when creating HTTP server
    Otherwise, the default HTTP server config may result in high latency.
    
    [1] https://www.extrahop.com/blog/tcp-nodelay-nagle-quickack-best-practices
    [2] https://eklitzke.org/the-caveats-of-tcp-nodelay
    03d49d0f25
  22. romanz force-pushed on Aug 25, 2024
  23. theStack approved
  24. theStack commented at 9:02 am on August 25, 2024: contributor
    re-ACK 03d49d0f25ab5660524d5ddd171de677a808b984
  25. tdb3 approved
  26. tdb3 commented at 7:21 pm on August 25, 2024: contributor

    ACK 03d49d0f25ab5660524d5ddd171de677a808b984

    Nice speedup! Saw a 28x speedup for a full block.

    Ran the test on a full block (ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000000000000b2ffb308ce769ad1c1b239c78553c4e3f931a74d5569.bin).

    27.1 release:

    0Requests per second:    5.08 [#/sec] (mean)
    1Time per request:       196.684 [ms] (mean)
    

    master (6d546336e800f7b8990fececab6bc08413f28690):

    0Requests per second:    5.28 [#/sec] (mean)
    1Time per request:       189.498 [ms] (mean)
    

    PR branch (03d49d0f25ab5660524d5ddd171de677a808b984):

    0Requests per second:    148.15 [#/sec] (mean)
    1Time per request:       6.750 [ms] (mean)
    

    Also repeated the test in the opening post (ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin) for 27.1, master, and the PR branch several times per binary. Although a significant amount of jitter between runs was seen, overall the PR branch had better time-per-request over master and 27.1.

  27. romanz commented at 4:04 pm on September 1, 2024: contributor
    Ping :)
  28. achow101 commented at 9:09 pm on September 3, 2024: member
    ACK 03d49d0f25ab5660524d5ddd171de677a808b984
  29. achow101 merged this on Sep 3, 2024
  30. achow101 closed this on Sep 3, 2024

  31. romanz deleted the branch on Sep 4, 2024
  32. romanz commented at 3:56 am on September 4, 2024: contributor
    Many thanks!

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-23 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me