diagnostics ff #114

Merged

navicore merged 1 commit from diagnostics-ff into main

2025-12-21 05:28:28 +00:00

navicore commented

2025-12-21 05:16:20 +00:00

(Migrated from github.com)

Changes made:

Cargo.toml: Added diagnostics feature (default enabled), signal-hook is now optional
scheduler.rs: Wrapped with #[cfg(feature = "diagnostics")]:
- StrandSlot, StrandRegistry structs and impls
- STRAND_REGISTRY static and strand_registry() function
- register() and unregister() calls in spawn path
- 4 registry-related tests
diagnostics.rs: Added #![cfg(feature = "diagnostics")] to disable entire module
watchdog.rs: Added #![cfg(feature = "diagnostics")] to disable entire module

Usage for benchmarks:

Build without diagnostics (no SystemTime::now() syscalls, no O(n) registry scans)

cargo build --release -p seq-runtime --no-default-features

Or in the compiler:

cargo build --release --no-default-features

Test results:

280 tests pass with diagnostics (default)
267 tests pass without diagnostics (13 diagnostic-related tests skipped)
Clippy clean both ways

Changes made: 1. Cargo.toml: Added diagnostics feature (default enabled), signal-hook is now optional 2. scheduler.rs: Wrapped with #[cfg(feature = "diagnostics")]: - StrandSlot, StrandRegistry structs and impls - STRAND_REGISTRY static and strand_registry() function - register() and unregister() calls in spawn path - 4 registry-related tests 3. diagnostics.rs: Added #![cfg(feature = "diagnostics")] to disable entire module 4. watchdog.rs: Added #![cfg(feature = "diagnostics")] to disable entire module Usage for benchmarks: # Build without diagnostics (no SystemTime::now() syscalls, no O(n) registry scans) cargo build --release -p seq-runtime --no-default-features # Or in the compiler: cargo build --release --no-default-features Test results: - 280 tests pass with diagnostics (default) - 267 tests pass without diagnostics (13 diagnostic-related tests skipped) - Clippy clean both ways

claude[bot] commented

2025-12-21 05:19:31 +00:00

(Migrated from github.com)

Code Review - PR #114: Diagnostics Feature Flag

This PR adds a feature flag to make diagnostics optional, allowing cleaner benchmarking without syscall overhead. Overall, this is a well-designed and well-executed change with proper documentation and testing.

✅ Strengths

Clean Feature Flag Design
- The diagnostics feature is correctly set as default, maintaining backwards compatibility
- Dependency on signal-hook is properly made optional via the feature flag
- Clear separation between production debugging tools and benchmarking requirements
Comprehensive Coverage
- All diagnostics-related code is properly gated with cfg(feature = diagnostics)
- Both the strand registry infrastructure and the signal handlers are correctly conditionally compiled
- Test coverage is excellent - 4 registry tests are properly feature-gated
Excellent Documentation
- The PR description clearly explains the motivation
- Inline documentation explains the performance impact
- Module-level docs explain the feature flag purpose
- Usage examples provided for building without diagnostics
Testing
- 280 tests pass with diagnostics (default)
- 267 tests pass without diagnostics (13 diagnostic tests correctly skipped)
- Clippy clean both ways

🔍 Minor Suggestions

scheduler.rs:293, 297 - Consider adding comments explaining why signal handler and watchdog setup can be safely skipped when diagnostics are disabled
Performance Impact Documentation - Consider adding a benchmark comparison showing actual performance delta with/without diagnostics
Test Coverage - Consider adding a test that verifies strand spawning works correctly with --no-default-features to ensure core scheduler functionality is unaffected

🔒 Security Considerations

Signal Handler Safety: Correctly uses signal-hook iterator API instead of unsafe direct calls
No Security Impact: Disabling diagnostics only removes debugging tools
No New Attack Surface: Feature flag only removes optional infrastructure

⚡ Performance Considerations

Eliminated Overhead:

No SystemTime::now() syscall on every strand spawn
No O(n) registry scan on spawn/unregister
No signal handler thread when disabled
No watchdog thread when disabled

Memory Impact: STRAND_REGISTRY not compiled saves ~16KB (default config)

✨ Recommendation

Approve with minor suggestions.

This PR is production-ready. The feature flag design is clean, the implementation is thorough, and testing is comprehensive. Minor suggestions are optional improvements for follow-up PRs.

Great work! 🚀

## Code Review - PR #114: Diagnostics Feature Flag This PR adds a feature flag to make diagnostics optional, allowing cleaner benchmarking without syscall overhead. Overall, this is a **well-designed and well-executed change** with proper documentation and testing. --- ### ✅ Strengths 1. **Clean Feature Flag Design** - The diagnostics feature is correctly set as default, maintaining backwards compatibility - Dependency on signal-hook is properly made optional via the feature flag - Clear separation between production debugging tools and benchmarking requirements 2. **Comprehensive Coverage** - All diagnostics-related code is properly gated with cfg(feature = diagnostics) - Both the strand registry infrastructure and the signal handlers are correctly conditionally compiled - Test coverage is excellent - 4 registry tests are properly feature-gated 3. **Excellent Documentation** - The PR description clearly explains the motivation - Inline documentation explains the performance impact - Module-level docs explain the feature flag purpose - Usage examples provided for building without diagnostics 4. **Testing** - 280 tests pass with diagnostics (default) - 267 tests pass without diagnostics (13 diagnostic tests correctly skipped) - Clippy clean both ways --- ### 🔍 Minor Suggestions 1. **scheduler.rs:293, 297** - Consider adding comments explaining why signal handler and watchdog setup can be safely skipped when diagnostics are disabled 2. **Performance Impact Documentation** - Consider adding a benchmark comparison showing actual performance delta with/without diagnostics 3. **Test Coverage** - Consider adding a test that verifies strand spawning works correctly with --no-default-features to ensure core scheduler functionality is unaffected --- ### 🔒 Security Considerations - Signal Handler Safety: Correctly uses signal-hook iterator API instead of unsafe direct calls - No Security Impact: Disabling diagnostics only removes debugging tools - No New Attack Surface: Feature flag only removes optional infrastructure --- ### ⚡ Performance Considerations Eliminated Overhead: - No SystemTime::now() syscall on every strand spawn - No O(n) registry scan on spawn/unregister - No signal handler thread when disabled - No watchdog thread when disabled Memory Impact: STRAND_REGISTRY not compiled saves ~16KB (default config) --- ### ✨ Recommendation **Approve with minor suggestions.** This PR is production-ready. The feature flag design is clean, the implementation is thorough, and testing is comprehensive. Minor suggestions are optional improvements for follow-up PRs. Great work! 🚀