Skip to content

Conversation

@cmcfarlen
Copy link
Contributor

No description provided.

@cmcfarlen cmcfarlen added this to the 10.2.0 milestone Oct 18, 2025
@cmcfarlen cmcfarlen self-assigned this Oct 18, 2025
@bryancall bryancall requested a review from moonchen October 20, 2025 21:51
@bryancall bryancall self-requested a review October 20, 2025 21:52
@cmcfarlen
Copy link
Contributor Author

There is one issue with this PR where Continuation.h externs this_ethread but it's inlined in EThread.h. See f590e4e

@bryancall bryancall requested a review from Copilot October 27, 2025 22:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.cc compilation 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.h and 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.

Comment on lines +47 to +57
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);
}
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@cmcfarlen cmcfarlen force-pushed the remove-inline-eventsystem branch from 57f903e to 1674d92 Compare November 3, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants