Skip to content

Optimize HTTP/2 upstream backpressure - #74

Closed
xchacha20-poly1305 wants to merge 1 commit into
TrustTunnel:masterfrom
xchacha20-poly1305:h2-pref
Closed

Optimize HTTP/2 upstream backpressure#74
xchacha20-poly1305 wants to merge 1 commit into
TrustTunnel:masterfrom
xchacha20-poly1305:h2-pref

Conversation

@xchacha20-poly1305

Copy link
Copy Markdown

Related Issue

Summary

Make HTTP/2 upstream back-pressure honor all three relevant limits — peer
session window, peer stream window, and local data-source queue capacity — so
callers stop over-committing bytes. Also split the TCP socket's single 128 KiB
constant into a 4 MiB queue threshold and a 256 KiB per-write cap to avoid
starving the upper layer on fast links.

Changes

  • core/src/http2_upstream.cpp: Http2Upstream::send() clamps length to
    available_to_send(id) and returns the actual bytes accepted instead of
    unconditionally reporting length.
  • net/src/http2.cpp:
    • http_session_available_to_write() returns min(session_window, stream_window, DATA_QUEUE_SIZE − pending) for non-zero stream IDs.
    • data_source_add() rejects writes that would overflow DATA_QUEUE_SIZE
      with NGHTTP2_ERR_BUFFER_ERROR before touching the evbuffer.
  • net/src/tcp_socket.cpp: Replace MAX_WRITE_BUFFER_LEN with
    MAX_WRITE_QUEUE_LEN (4 MiB, used by tcp_socket_available_to_write) and
    MAX_SINGLE_WRITE_LEN (256 KiB, passed to
    bufferevent_set_max_single_write).
  • net/test/test_http2.cpp (+ net/CMakeLists.txt): New unit test covering
    both stream-window and session-window back-pressure transitions.
  • common/src/event_loop.cpp, net/src/os_tunnel.cpp,
    tcpip/test/test_vpn_packet_pool.cpp: Replace std::unique_ptr<T>{new T{}}
    with std::make_unique<T>().

Tests

  • New or updated tests cover the changes (net/test/test_http2.cpp
    exercises stream-window exhaustion, queue-cap exhaustion, and recovery via
    WINDOW_UPDATE on both the stream and the connection)
  • All existing tests pass (make test)
  • Benchmarks updated — comment /bench on this PR to dispatch the
    endpoint-bench workflow (.github/workflows/bench.yml), then paste the
    results into the Benchmarks section below

Benchmarks

Need to run /bench command. Skip for now.

Scenario master this PR Δ
HTTP/2 upstream throughput (1 stream, high BDP)
HTTP/2 upstream throughput (N streams)
TCP write latency (p50 / p99)

Checklist

  • Documentation updated (if applicable)
  • CHANGELOG.md updated (if user-visible change)
  • No secrets or credentials committed

@anyakushin anyakushin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, @xchacha20-poly1305, thanks for your effort and sorry for a long delay.

This PR has been reviewed and the team have some concerns about it:

  • The session window is not respected by design: the TrustTunnel multiplexes unrelated TCP connections via different HTTP/2 streams, so these streams should be unrelated too. Respecting the session window would just result in re-coupling these connections and distort the backpressure. If there is one stream that advertises a big window and starves, it will fill the whole session window that will starve all other TCP connections. So this is an unacceptable change.
  • The local data-source queue capacity limit is not required - the read from TUN is stopped if the remove window is 0 so this will be just an additional unnecessary protection that will never be triggered.
  • The 4 MiB queue threshold change requires the evidences. I've done some bench tests and don't see any performance improvements even with relatively big BDP. Moreover, the 4MiB is big enough to exceed the network extension memory limit's for apple devices, so even with concrete evidences this can't be a hardcoded default for all platforms.

Thus, this PR can't be accepted in the current form, respecting session window is strongly unacceptable change by design and other changes require evidences.

Thanks again for your effort, any following work on this PR and finding the evidences would be very appreciated, i personally failed to find any myself.

P.s. husi is awesome

@xchacha20-poly1305

Copy link
Copy Markdown
Author

Thanks for the explanation. I agree with your concerns. I’m not deeply familiar with C++ and this part of TrustTunnel’s flow-control design, and my change is only based on observing a performance issue and trying to improve it. From your explanation, I understand why this approach is not acceptable for TrustTunnel’s design.

I don’t plan to continue pushing this PR. I hope the TrustTunnel team can find a better optimization strategy that fits the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants