feat: add benchmarks + socket optimizations #6
Conversation
WalkthroughThis update introduces a comprehensive benchmarking and testing infrastructure, including a GitHub Actions workflow for automated Rust benchmarks and extensive platform-specific and integration tests for socket enumeration. The public API is slightly expanded by re-exporting the Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Runner as GitHub Actions Runner
participant Cargo
participant PR as Pull Request
GitHub->>Runner: Trigger benchmark workflow (push/PR/schedule)
Runner->>Runner: Checkout code
Runner->>Runner: Install Rust toolchain
Runner->>Runner: Cache dependencies/build
Runner->>Cargo: Run `cargo bench`
Cargo-->>Runner: Benchmark results
alt Triggered by Pull Request
Runner->>PR: Post benchmark results as comment
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/socket/platform.rs (1)
605-615: Unit-test still over-strict → causes CI failureCI shows
Failed to get process info …originating fromassert!(process_info.is_some()).
get_process_infolegitimately returnsNonefor kernel / root PIDs, sandboxed daemons, etc.- assert!( - process_info.is_some(), - "Failed to get process info for PID {pid}" - ); + // Some PIDs (e.g. kernel threads or permission-restricted processes) + // deliberately do not expose details – treat this as non-fatal. + if process_info.is_none() { + log::debug!("process info unavailable for pid {pid}"); + continue; + }This will stabilise the test while still exercising the happy-path.
🧹 Nitpick comments (9)
src/socket/platform.rs (2)
613-615: Remove commented-out assertions instead of keeping dead codeLeaving commented code (
// assert!(info.cmdline.is_some());) clutters the test and invites bit-rot.
Either restore with conditional logic or delete it.
633-635: Great relaxation on wildcard ports – factor out helper to DRYThe repeated pattern
assert!(socket.local_addr.port() > 0 || socket.local_addr.ip().is_unspecified());now appears four times.
Consider extracting a small helper:fn has_valid_port(addr: &SocketAddr) -> bool { addr.port() > 0 || addr.ip().is_unspecified() }and using
assert!(has_valid_port(&socket.local_addr));to keep the tests concise.Also applies to: 703-704, 716-717, 728-729
tests/platform/mod.rs (1)
1-7: Platform gate is correct – mind themod.rsdeprecationConditional inclusion is on point.
mod.rsis still accepted but Rust 2024 edition plans to discourage it; consider renaming totests/platform.rswith sibling files if you migrate later..github/workflows/benchmark.yaml (1)
47-47: Remove trailing spacesApply this diff to remove trailing spaces:
- const output = fs.readFileSync('bench_output.txt', 'utf8'); - + const output = fs.readFileSync('bench_output.txt', 'utf8'); +tests/socket_discovery.rs (2)
127-127: Remove unreachable patternThis catch-all pattern is unreachable since line 96 already validates that the protocol is either TCP or UDP.
Apply this diff to remove the unreachable code:
- _ => {}
247-247: Simplify duration calculationThe conversion to u32 is unnecessary as Duration can be divided by usize directly.
Apply this diff to simplify:
- let avg_duration = durations.iter().sum::<Duration>() / u32::try_from(durations.len()).unwrap(); + let avg_duration = durations.iter().sum::<Duration>() / durations.len() as u32;tests/macos_tests.rs (2)
92-92: Handle or explicitly ignore connection resultThe connection attempt returns a Result that should be handled or explicitly ignored.
Apply this diff to explicitly handle the result:
- let connector = std::net::TcpStream::connect(("127.0.0.1", listening_port)); + let _connector = std::net::TcpStream::connect(("127.0.0.1", listening_port)).ok();
203-203: Simplify duration calculationThe conversion is unnecessary and could panic on overflow.
Apply this diff to simplify:
- let avg_duration = durations.iter().sum::<Duration>() / u32::try_from(durations.len()).unwrap(); + let avg_duration = durations.iter().sum::<Duration>() / durations.len() as u32;tests/linux_tests.rs (1)
188-188: Use idiomatic empty checkApply this diff to use the more idiomatic approach:
- assert!(state_counts.len() > 0, "Should find various TCP states"); + assert!(!state_counts.is_empty(), "Should find various TCP states");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/benchmark.yaml(1 hunks)Cargo.toml(1 hunks)benches/socket_performance.rs(1 hunks)src/lib.rs(1 hunks)src/socket/platform.rs(5 hunks)tests/integration/mod.rs(1 hunks)tests/linux_tests.rs(1 hunks)tests/macos_tests.rs(1 hunks)tests/platform/mod.rs(1 hunks)tests/socket_discovery.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/socket/platform.rs (3)
src/socket/tcp.rs (1)
local_addr(127-129)src/socket/udp.rs (1)
local_addr(120-122)src/socket/socket.rs (1)
local_addr(46-46)
tests/socket_discovery.rs (2)
src/socket/platform.rs (5)
get_sockets_info(563-580)get_sockets_info(773-830)get_sockets_info(833-863)get_sockets_info(1349-1376)get_sockets_info(1521-1553)src/socket/socket.rs (3)
protocol(213-216)for_process(443-448)listening_on(454-459)
🪛 actionlint (1.7.7)
.github/workflows/benchmark.yaml
21-21: the runner of "actions-rs/toolchain@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/benchmark.yaml
[error] 47-47: trailing spaces
(trailing-spaces)
🪛 GitHub Actions: Rust Tests
src/socket/platform.rs
[error] 605-605: Test failure: Failed to get process info for PID 2408 in test_process_info_retrieval.
🔇 Additional comments (6)
Cargo.toml (1)
56-60: Bench manifest LGTM – just double-checkcriterioncompile flags
criterion 0.5is the right crate andharness = falseis required.
Nothing to fix here, but consider adding a[profile.bench]section (e.g.debug = false) if you need fully-optimised numbers.src/lib.rs (1)
44-45: Public API surface changed — bump the crate versionRe-exporting
Protocolexposes a new public item at the crate root.
According to Cargo semver rules this is a non-breaking, yet public-API addition.
Please bumpversionto0.1.1(or0.2.0if you prefer feature bumps to live on a minor) before publishing.tests/integration/mod.rs (1)
1-3: Integration test module scaffold looks goodThe module compiles as its own integration crate (
tests/integration), and re-exportssocket_discovery. No issues spotted.tests/socket_discovery.rs (2)
10-53: Well-structured socket discovery testGood approach using warnings instead of hard failures for timing-dependent socket visibility. The test handles platform variations gracefully.
55-80: Clean filtering testsGood coverage of protocol filtering and process association verification.
benches/socket_performance.rs (1)
1-242: Comprehensive benchmark suiteExcellent coverage of various socket operations with platform-specific optimizations. The benchmark structure follows Criterion best practices with proper use of black_box and benchmark groups.
…otations Co-authored-by: chad.nehemiah94 <[email protected]>
…ents-on-pr-0bfd
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CODERABBIT_FIXES.md (1)
28-30: Minor punctuation tweak for clarity
Add a comma after “function” to improve readability.- - Added `#[allow(dead_code)]` attribute to the function since it may be used in future implementations or by external consumers of the library. + - Added `#[allow(dead_code)]` attribute to the function, since it may be used in future implementations or by external consumers of the library.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CODERABBIT_FIXES.md(1 hunks)src/interface/monitor.rs(3 hunks)src/routing.rs(1 hunks)src/socket/monitor.rs(1 hunks)src/socket/platform.rs(7 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/socket/monitor.rs
- src/routing.rs
- src/interface/monitor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/socket/platform.rs
🧰 Additional context used
🪛 LanguageTool
CODERABBIT_FIXES.md
[uncategorized] ~29-~29: Possible missing comma found.
Context: ... #[allow(dead_code)] attribute to the function since it may be used in future implemen...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (1)
CODERABBIT_FIXES.md (1)
49-54: Verification checklist looks thorough – nice work
The explicit mention ofcargo check,clippy,test, andfmtprovides confidence that the fixes were validated across the standard Rust toolchain. No further action needed.
Benchmark Results |
Benchmark Results |
Benchmark Results |
Benchmark Results |
Summary by CodeRabbit
New Features
Protocoltype at the crate root for easier access.Bug Fixes
Tests
Chores
Refactor
serdetoserde-supportfor conditional serde support across the codebase.