Remove eventsystem Inline.cc and cleanup private headers. #12581
Conversation
|
There is one issue with this PR where Continuation.h externs |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the eventsystem Inline.cc file and eliminates private header dependencies by converting inline functions to regular implementations and consolidating code that was previously split across private headers (P_*.h) into public headers or implementation files.
Key changes:
- Removed
Inline.cccompilation unit that previously forced inline function instantiation - Moved inline function implementations from private headers to either public headers (as inline) or implementation files (as regular functions)
- Updated all includes to use public headers instead of private
P_EventSystem.hand related headers - Deleted several private headers that are no longer needed (
P_VConnection.h,P_VIO.h,P_IOBuffer.h, etc.)
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/iocore/eventsystem/Inline.cc |
Deleted - no longer needed for forcing inline function instantiation |
src/iocore/eventsystem/P_EventSystem.h |
Deleted - consolidated into public headers |
src/iocore/eventsystem/VIO.cc |
New file containing VIO method implementations previously in P_VIO.h |
src/iocore/eventsystem/IOBuffer.cc |
Moved inline functions from P_IOBuffer.h to regular implementations |
src/iocore/eventsystem/UnixEventProcessor.cc |
Moved EventProcessor methods from P_UnixEventProcessor.h |
src/iocore/eventsystem/UnixEThread.cc |
Moved EThread scheduling methods from P_UnixEThread.h |
include/iocore/eventsystem/*.h |
Converted selected functions to inline in public headers |
| Various source files | Updated to include public headers instead of private P_*.h files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| VIO * | ||
| vc_do_io_write(VConnection *vc, Continuation *cont, int64_t nbytes, MIOBuffer *buf, int64_t offset) | ||
| { | ||
| IOBufferReader *reader = buf->alloc_reader(); | ||
|
|
||
| if (offset > 0) { | ||
| reader->consume(offset); | ||
| } | ||
|
|
||
| return vc->do_io_write(cont, nbytes, reader, true); | ||
| } |
There was a problem hiding this comment.
This function vc_do_io_write appears to be duplicated from the removed P_VConnection.h. Consider whether this should be moved to a shared location or if it's intentionally file-specific. The same implementation was in the deleted header file, suggesting it might be used elsewhere.
|
|
||
| int thread_max_heartbeat_mseconds = THREAD_MAX_HEARTBEAT_MSECONDS; | ||
| int thread_max_heartbeat_mseconds = THREAD_MAX_HEARTBEAT_MSECONDS; | ||
| const ink_hrtime DELAY_FOR_RETRY = HRTIME_MSECONDS(10); |
There was a problem hiding this comment.
The constant DELAY_FOR_RETRY is being defined here but was previously in P_UnixEThread.h. If this constant is used in other translation units, this change could cause linker errors due to multiple definitions, or undefined references if those files haven't been updated to include the necessary declaration.
cmcfarlen
force-pushed
the
remove-inline-eventsystem
branch
from
57f903e to
1674d92
Compare
There was a problem hiding this comment.
✅ Approval: Comprehensive Testing Complete
I've completed extensive testing of this PR and recommend approval. The eventsystem refactoring is well-executed with measurable improvements and no regressions.
Test Results Summary
Build Performance
- Build time: 57.4s → 48.4s (-16% improvement)
- Binary size: 120MB → 118MB (-1.7% reduction)
- Compiler warnings: 0 (no new warnings)
- All build configurations tested: ✅ ASAN, Release, Debug
Runtime Performance (Release builds, proper cache configuration)
Configuration: h2load, 1M requests, 1000 clients, HTTP/1.1, 100% cache hits, 10 cache volumes @ 10% each
- Master average: 545,310 req/s (4 runs)
- PR average: 544,953 req/s (4 runs)
- Difference: -0.07% (within measurement noise)
- Conclusion: Performance is equivalent
Unit Tests (ctest)
- Master: 64/64 tests passed (100%)
- PR: 64/64 tests passed (100%)
- Result: ✅ No test regressions
Critical tests verified:
test_EventSystem(core eventsystem - key for this PR)test_IOBuffer(IOBuffer operations)test_MIOBufferWriter(buffer writing)- All other core unit tests passing
Functional Testing
- ✅ 34,495 event loops processed successfully
- ✅ 2,077 events handled correctly
- ✅ All eventsystem components working (Event, EThread, IOBuffer, VIO, Continuation)
- ✅ No ASAN errors (AddressSanitizer clean)
- ✅ No crashes, assertions, or fatal errors
- ✅ Plugins fully compatible (header_rewrite, cache_range_requests tested)
Code Quality
- Net code reduction: -1,517 lines
- Files changed: 97 files
- Private headers removed: 9 (P_EventSystem.h, P_IOBuffer.h, P_VIO.h, etc.)
- Inline.cc removed: No longer needed
- Refactoring quality: Pure code movement, no logic changes
Why This PR Improves the Codebase
- Better Performance: 16% faster builds, equivalent runtime performance
- Smaller Binaries: 1.7-3% reduction in binary sizes
- Better Compiler Optimization: Hot path functions now inline (e.g.,
this_ethread()) - Cleaner API: Public headers self-contained, no private header dependencies
- More Maintainable: Removed unnecessary compilation unit, better code organization
Technical Analysis
The PR successfully:
- Removes the
Inline.cccompilation unit (no longer needed) - Moves inline functions from private headers to public headers
- Deletes 9 private headers that are no longer required
- Maintains complete functional equivalence
- Improves build performance through better compilation unit organization
- Enables better compiler optimization (inline functions in headers)
Recommendation
✅ APPROVE - This is an excellent refactoring that demonstrates:
- Measurable improvements (build time, binary size)
- No functional regressions
- No performance regressions
- Better code organization
- Thorough testing validation
The eventsystem refactoring is safe, beneficial, and ready to merge.
Testing Environment:
- OS: Fedora 43, Linux 6.17.6
- Compiler: GCC 15.2.1
- Test duration: ~3 hours
- Configurations tested: ASAN, Release, Debug
3 participants
No description provided.