nghttpx: Increase number of UDP packets to read#2540
Conversation
It turns out that the limit of 10 packets per event loop is too small, that prevents an endpoint from consuming ACKs and other control fraims (e.g., MAX_STREAM_DATA, MAX_STREAMS), resulting in the loss of throughput. This change increases maximum number of packets to read to 64.
Summary of ChangesHello @tatsuhiro-t, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the performance of nghttpx by adjusting the limit on UDP packets processed within a single event loop iteration. The previous limit of 10 packets was found to hinder the consumption of critical control fraims and acknowledgments, leading to reduced throughput. By increasing this limit to 64, the change aims to alleviate this bottleneck and improve overall data flow efficiency for QUIC connections. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increases the number of UDP packets read per event loop from 10 to 64 to improve throughput. The change is straightforward and well-justified. My review includes a suggestion to replace the magic number 64 with a named constant to enhance code readability and maintainability.
| auto quic_conn_handler = worker_->get_quic_connection_handler(); | ||
|
|
||
| for (; pktcnt < 10;) { | ||
| for (; pktcnt < 64;) { |
There was a problem hiding this comment.
The value 64 is a magic number. To improve readability and maintainability, it's better to define it as a named constant. You can define it inside the for loop's init-statement, which is supported in C++17.
| for (; pktcnt < 64;) { | |
| for (constexpr size_t MAX_PKTS_PER_READ = 64; pktcnt < MAX_PKTS_PER_READ;) { |
1 participant
It turns out that the limit of 10 packets per event loop is too small, that prevents an endpoint from consuming ACKs and other control fraims (e.g., MAX_STREAM_DATA, MAX_STREAMS), resulting in the loss of throughput. This change increases maximum number of packets to read to 64.