Skip to content

Conversation

ghalliday
Copy link
Member

@ghalliday ghalliday commented Sep 23, 2025

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

Copy link

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-34816

Jirabot Action Result:
Workflow Transition To: Merge Pending
Additional PR: #20406

Copy link
Contributor

@Copilot 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 pull request adds support for io_uring to jlib and roxie sender, implementing asynchronous I/O operations for improved performance on Linux systems. The changes introduce a new IAsyncProcessor interface for handling asynchronous operations and integrate it with the existing socket communication infrastructure.

Key Changes:

  • Added io_uring support through new IAsyncProcessor interface and URingProcessor implementation
  • Enhanced CTcpSender with asynchronous write capabilities and state machine management
  • Integrated io_uring support into Roxie's TCP sender with configurable enablement

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
vcpkg.json.in Added liburing dependency for Linux platforms
system/jlib/jiouring.hpp New interface definitions for async processor and callback
system/jlib/jiouring.cpp Complete io_uring implementation with threaded/unthreaded variants
system/jlib/CMakeLists.txt Build configuration for liburing dependency
system/jlib/jmutex.hpp Added tryEnter() method to CriticalSection classes
system/jlib/jlib.hpp Added common size constants (oneKB, oneMB, oneGB)
system/jlib/jstats.cpp Updated to use new size constants
system/jlib/jfile.hpp Updated size constants and includes
system/jlib/jencrypt.cpp Improved error handling with OpenSSL error details
system/security/securesocket/socketutils.hpp Enhanced CSocketTarget with async write support and state machine
system/security/securesocket/socketutils.cpp Implemented async write functionality with queuing and reconnection logic
roxie/udplib/tcptrs.cpp Integrated io_uring support with Roxie TCP sender
roxie/roxiemem/roxiemem.cpp Added memory registration for io_uring fixed buffers
roxie/ccd/ccdqueue.cpp Added configuration option for io_uring usage
testing/unittests/jlibtests2.cpp Added comprehensive unit tests for io_uring functionality

Comment on lines +116 to +122
for (;;)
{
io_uring_sqe *sqe = io_uring_get_sqe(&ring);
if (sqe)
return sqe;
// MORE: If the buffer is full we need to wait for some completions and try again
MilliSleep(1);
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The busy-wait loop with MilliSleep(1) is inefficient and could cause unnecessary CPU usage. Consider implementing a proper blocking mechanism or exponential backoff strategy instead of a fixed 1ms sleep.

Suggested change
for (;;)
{
io_uring_sqe *sqe = io_uring_get_sqe(&ring);
if (sqe)
return sqe;
// MORE: If the buffer is full we need to wait for some completions and try again
MilliSleep(1);
unsigned sleepMs = 1;
const unsigned maxSleepMs = 64;
for (;;)
{
io_uring_sqe *sqe = io_uring_get_sqe(&ring);
if (sqe)
return sqe;
// Exponential backoff to reduce CPU usage while waiting for an available SQE
MilliSleep(sleepMs);
if (sleepMs < maxSleepMs)
sleepMs *= 2;

Copilot uses AI. Check for mistakes.

Comment on lines +216 to +218
//MORE: Check that the buffer does not overlap a 1GB boundary
const byte * start = static_cast<const byte *>(buf);
return (start >= startLockedMemory) && (start + len <= endLockedMemory);
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The comment indicates that buffer boundary checking is missing, but the code doesn't validate that buffers don't cross 1GB boundaries as required by io_uring fixed buffer operations. This could cause runtime failures when buffers span multiple registered memory blocks.

Suggested change
//MORE: Check that the buffer does not overlap a 1GB boundary
const byte * start = static_cast<const byte *>(buf);
return (start >= startLockedMemory) && (start + len <= endLockedMemory);
// Ensure the buffer does not overlap a 1GB boundary
const byte * start = static_cast<const byte *>(buf);
const byte * end = start + len - 1;
// Check that start and end are within the same 1GB block
bool withinSameBlock = (((memsize_t)start) & ~(oneGB - 1)) == (((memsize_t)end) & ~(oneGB - 1));
return (start >= startLockedMemory) && (end < endLockedMemory) && withinSameBlock;

Copilot uses AI. Check for mistakes.

}
else
{
OERRLOG("Short write on socket %u of %zu", result, request.len);
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The format specifiers are incorrect. result is an int but uses %u (unsigned), and request.len is size_t but uses %zu which may not be supported on all platforms. Use %d for result and cast request.len to unsigned with %u.

Suggested change
OERRLOG("Short write on socket %u of %zu", result, request.len);
OERRLOG("Short write on socket %d of %u", result, (unsigned)request.len);

Copilot uses AI. Check for mistakes.

Comment on lines +584 to +598
for(;;)
{
//If reconnecting, drop the packets until the connection has succeeded.
if (unlikely((state == State::Aborting) || (state == State::Reconnecting)))
{
sender.releaseBuffer(ownedBuffer);
return;
}

//Ensure there is space to record the next item to send..
//Maybe this should always expand rather than block???
if (likely(numRequests != maxQueueDepth))
break;

waitForRequestSpace(block);
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The infinite loop with break conditions scattered throughout makes the control flow difficult to follow. Consider restructuring this into a while loop with a clear condition or extracting the logic into separate helper methods.

Suggested change
for(;;)
{
//If reconnecting, drop the packets until the connection has succeeded.
if (unlikely((state == State::Aborting) || (state == State::Reconnecting)))
{
sender.releaseBuffer(ownedBuffer);
return;
}
//Ensure there is space to record the next item to send..
//Maybe this should always expand rather than block???
if (likely(numRequests != maxQueueDepth))
break;
waitForRequestSpace(block);
// If reconnecting or aborting, drop the packets until the connection has succeeded.
if (unlikely((state == State::Aborting) || (state == State::Reconnecting)))
{
sender.releaseBuffer(ownedBuffer);
return;
}
// Ensure there is space to record the next item to send.
// Maybe this should always expand rather than block???
while (numRequests == maxQueueDepth)
{
waitForRequestSpace(block);
// After waiting, check state again in case it changed.
if (unlikely((state == State::Aborting) || (state == State::Reconnecting)))
{
sender.releaseBuffer(ownedBuffer);
return;
}

Copilot uses AI. Check for mistakes.

{
sender.setAsyncProcessor(asyncSender);

//MORE: I am not sure if this is actually worthwhile - need to perform some performance tests
Copy link
Preview

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

This TODO comment indicates uncertainty about the performance benefits. Either conduct the performance tests and update the code accordingly, or document the expected benefits and conditions under which this optimization should be used.

Suggested change
//MORE: I am not sure if this is actually worthwhile - need to perform some performance tests
// Register Roxie memory with the asyncSender to enable optimized memory management when using io_uring.
// This is expected to improve performance by allowing the async processor to efficiently handle Roxie-managed buffers.
// Enable this registration when using io_uring-based async senders, especially in high-throughput scenarios.

Copilot uses AI. Check for mistakes.

@ghalliday
Copy link
Member Author

@mckellyln I closed the previous PR and opened a clean one. Commits have been squashed because the steps didn't clarify the work.
This is a minimal implementation supporting partial writes. Future PRs will include using writev() to clear multiple queued sends at the same time, and implementing async connect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant