7978: docs(runtime): document FD table pre-warming workaround#79
7978: docs(runtime): document FD table pre-warming workaround#79martin-augment wants to merge 4 commits intomasterfrom
Conversation
Summary of ChangesHello, 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 introduces a significant performance optimization for Tokio applications running on Linux by providing a mechanism to pre-warm the kernel's file descriptor table. This addresses a known issue where lazy FD table growth can cause substantial latency spikes in multi-threaded environments. The changes include a new example demonstrating the workaround and updated runtime documentation to guide users on implementing this performance tuning technique. 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. Footnotes
|
WalkthroughThis pull request introduces a new example demonstrating file descriptor (FD) table pre-warming on Linux systems. A new file ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![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 introduces documentation and an example for a workaround to pre-warm the file descriptor table on Linux, which can help avoid latency spikes in multi-threaded applications. The changes include a new example file, updates to Cargo.toml to support it, and additions to the runtime module's documentation. My review focuses on the correctness and safety of the new example code. I've suggested a small improvement to ensure type safety at the FFI boundary.
| use std::os::unix::io::{FromRawFd, OwnedFd}; | ||
|
|
||
| // Open /dev/null to get a base FD. | ||
| let dev_null = std::fs::File::open("/dev/null")?; | ||
|
|
||
| // Use F_DUPFD_CLOEXEC to duplicate to a slot >= target. This forces the | ||
| // kernel to expand the FD table in a single syscall, without clobbering | ||
| // existing FDs. CLOEXEC prevents leaking to child processes. | ||
| let raw = unsafe { | ||
| libc::fcntl( | ||
| std::os::unix::io::AsRawFd::as_raw_fd(&dev_null), | ||
| libc::F_DUPFD_CLOEXEC, | ||
| target, | ||
| ) |
There was a problem hiding this comment.
The target parameter is a u32, but fcntl's third argument for F_DUPFD_CLOEXEC is a c_int (i32 on Linux). Passing a u32 value that is larger than i32::MAX will be interpreted as a negative number, causing fcntl to fail. It's safer to validate the input and use the correct type.
use std::convert::TryInto;
use std::os::unix::io::{FromRawFd, OwnedFd};
let target = target.try_into().map_err(|e| {
std::io::Error::new(std::io::ErrorKind::InvalidInput, e)
})?;
// Open /dev/null to get a base FD.
let dev_null = std::fs::File::open("/dev/null")?;
// Use F_DUPFD_CLOEXEC to duplicate to a slot >= target. This forces the
// kernel to expand the FD table in a single syscall, without clobbering
// existing FDs. CLOEXEC prevents leaking to child processes.
let raw = unsafe {
libc::fcntl(
std::os::unix::io::AsRawFd::as_raw_fd(&dev_null),
libc::F_DUPFD_CLOEXEC,
target,
)There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! FFI with libc uses c_int, which is i32 in Rust. Using other integer types may work or not. It is better to use the same types as the libc's APIs.
|
Code Review This is a well-focused documentation/example PR. Overall the changes are clean and correct. Strengths:
Issues and Suggestions:
Summary: Items 1 and 2 are minor style nits. Item 4 is the most practically impactful for users deploying in containers. The core logic, safety model, and documentation are all correct. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| libc::F_DUPFD_CLOEXEC, | ||
| target, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Wrong type passed to variadic FFI fcntl call
Low Severity
The target parameter is typed u32 but is passed directly as a variadic argument to libc::fcntl, which expects c_int (i32) for F_DUPFD_CLOEXEC. Passing a mismatched type to a C variadic function is undefined behavior per the C standard. While harmless for the hardcoded value of 10,000, this example is designed to be copied by users who might supply larger values — any u32 above i32::MAX would be UB. The parameter type (and FD_TARGET const) would be more correct as libc::c_int or i32.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Bugbot AI reviewer is correct! FFI with libc uses c_int, which is i32 in Rust. Using other integer types may work or not. It is better to use the same types as the libc's APIs.
🤖 Augment PR SummarySummary: Adds docs + a runnable example for a Linux workaround that pre-warms the kernel FD table to avoid RCU-related stalls on Tokio worker threads. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| libc::fcntl( | ||
| std::os::unix::io::AsRawFd::as_raw_fd(&dev_null), | ||
| libc::F_DUPFD_CLOEXEC, | ||
| target, |
There was a problem hiding this comment.
Because libc::fcntl is varargs, passing target as a u32 can be ABI-incorrect on some platforms; consider casting to the exact expected C type for F_DUPFD_CLOEXEC (typically libc::c_int) to avoid UB in this unsafe call.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! FFI with libc uses c_int, which is i32 in Rust. Using other integer types may work or not. It is better to use the same types as the libc's APIs.
| if let Err(e) = prewarm_fd_table(FD_TARGET) { | ||
| eprintln!("Warning: failed to pre-warm FD table: {e}"); | ||
| } else { | ||
| println!("FD table pre-warmed successfully."); |
There was a problem hiding this comment.
On non-Linux targets prewarm_fd_table is a no-op that returns Ok(()), so this “pre-warmed successfully” message can be misleading when the example is run on macOS/Windows.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! The whole example does not make sense for non-Linux runs. It would be better to return early in main() with a message printed on the stdout that the example does nothing on non-Linux. Currently it prints success messages even when it did nothing and this may confuse the users.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/prewarm-fd-table.rs`:
- Around line 65-69: The code prints "FD table pre-warmed successfully." even on
non-Linux where prewarm_fd_table(FD_TARGET) is a no-op; change the logic so the
success message is only emitted when the pre-warm actually ran (e.g., guard the
println! with cfg!(target_os = "linux") or adjust prewarm_fd_table to return a
boolean/enum indicating it performed work and print only when that indicates
success). Update the branch around the call to prewarm_fd_table(FD_TARGET) (and
the associated success println!) to rely on that condition so non-Linux
platforms no longer show a false success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 338934f7-af4d-4511-8ea7-baa4c9afff15
📒 Files selected for processing (4)
examples/Cargo.tomlexamples/prewarm-fd-table.rsspellcheck.dictokio/src/runtime/mod.rs
| if let Err(e) = prewarm_fd_table(FD_TARGET) { | ||
| eprintln!("Warning: failed to pre-warm FD table: {e}"); | ||
| } else { | ||
| println!("FD table pre-warmed successfully."); | ||
| } |
There was a problem hiding this comment.
Non-Linux path reports a false success message.
On non-Linux, prewarm_fd_table is a no-op but this branch still prints FD table pre-warmed successfully. which is misleading.
Suggested adjustment
fn main() {
const FD_TARGET: u32 = 10_000;
println!("Pre-warming FD table to {FD_TARGET} entries...");
- if let Err(e) = prewarm_fd_table(FD_TARGET) {
- eprintln!("Warning: failed to pre-warm FD table: {e}");
- } else {
- println!("FD table pre-warmed successfully.");
- }
+ #[cfg(target_os = "linux")]
+ {
+ if let Err(e) = prewarm_fd_table(FD_TARGET) {
+ eprintln!("Warning: failed to pre-warm FD table: {e}");
+ } else {
+ println!("FD table pre-warmed successfully.");
+ }
+ }
+ #[cfg(not(target_os = "linux"))]
+ {
+ let _ = prewarm_fd_table(FD_TARGET);
+ println!("Skipping FD table pre-warm (Linux-only workaround).");
+ }
// Build the runtime *after* pre-warming.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Err(e) = prewarm_fd_table(FD_TARGET) { | |
| eprintln!("Warning: failed to pre-warm FD table: {e}"); | |
| } else { | |
| println!("FD table pre-warmed successfully."); | |
| } | |
| #[cfg(target_os = "linux")] | |
| { | |
| if let Err(e) = prewarm_fd_table(FD_TARGET) { | |
| eprintln!("Warning: failed to pre-warm FD table: {e}"); | |
| } else { | |
| println!("FD table pre-warmed successfully."); | |
| } | |
| } | |
| #[cfg(not(target_os = "linux"))] | |
| { | |
| let _ = prewarm_fd_table(FD_TARGET); | |
| println!("Skipping FD table pre-warm (Linux-only workaround)."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/prewarm-fd-table.rs` around lines 65 - 69, The code prints "FD table
pre-warmed successfully." even on non-Linux where prewarm_fd_table(FD_TARGET) is
a no-op; change the logic so the success message is only emitted when the
pre-warm actually ran (e.g., guard the println! with cfg!(target_os = "linux")
or adjust prewarm_fd_table to return a boolean/enum indicating it performed work
and print only when that indicates success). Update the branch around the call
to prewarm_fd_table(FD_TARGET) (and the associated success println!) to rely on
that condition so non-Linux platforms no longer show a false success.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The whole example does not make sense for non-Linux runs. It would be better to return early in main() with a message printed on the stdout that the example does nothing on non-Linux. Currently it prints success messages even when it did nothing and this may confuse the users.
2 participants
7978: To review by AI