Skip to content

Epoll: use busy poll cycle limit - #595

Open
romange wants to merge 1 commit into
masterfrom
epoll-busy-poll-limit
Open

Epoll: use busy poll cycle limit#595
romange wants to merge 1 commit into
masterfrom
epoll-busy-poll-limit

Conversation

@romange

@romange romange commented Jun 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • replace EpollProactor's fixed spin-loop threshold with the shared busy-poll cycle budget
  • reset the busy-poll window after real CPU or I/O work, matching UringProactor behavior
  • keep zero-timeout epoll probes from resetting idle accounting so they count against the spill budget

Testing

  • Built target: fibers2

Summary by CodeRabbit

  • Refactor
    • Event-loop busy/idle logic reworked to reduce unnecessary spinning and refine idle/termination behavior.
    • Startup spin calibration reduced for faster, more efficient startup.
  • Performance
    • Queue cell layout adjusted for improved cache alignment and throughput.
  • Behavior
    • Task exceptions now propagate instead of being locally caught.
  • New Features
    • Added a lightweight tasklet type and a single-consumer dequeue API.
  • Tests
    • Added unit test validating single-consumer dequeue behavior.

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Proactor loops: remove spin_loops; compute should_poll from CycleClock delta; add ResetBusyPoll() at task/completion/scheduler checkpoints; adjust idle accounting and termination flow; inline ProactorBase ResetBusyPoll and tweak init; plus related mpmc alignment and fiberqueue/tasklet changes.

Changes

Busy-polling and idle-task event-loop redesign

Layer / File(s) Summary
kqueue timespec initialization
util/fibers/epoll_proactor.cc
kqueue EpollWait now constructs the timespec timeout argument using designated-initializer syntax.
Remove spin_loops and compute should_poll
util/fibers/epoll_proactor.cc
Deleted spin_loops gating; compute should_poll from CycleClock::Now() - busy_poll_start_cycle() < busy_poll_cycle_limit_; call IdleEnd(start_cycle) only when timeout != 0.
ResetBusyPoll() integration points
util/fibers/epoll_proactor.cc
Call ResetBusyPoll() after running task batches, before dispatching completions, and around L2/NORMAL scheduler runs to reset busy-poll state at transitions.
BACKGROUND idle/termination and RunOnIdleTasks
util/fibers/epoll_proactor.cc
On BACKGROUND HAS_ACTIVE call ResetBusyPoll(); use RunOnIdleTasks() to optionally continue; otherwise pause briefly only if should_poll before scheduler->DestroyTerminated().
ProactorBase API and init tweaks
util/fibers/proactor_base.h, util/fibers/proactor_base.cc
Inline ResetBusyPoll() in header, remove kMaxSpinLimit, and change ModuleInit() calibration Pause(...) call to Pause(5).
Uring should_poll computation
util/fibers/uring_proactor.cc
Uring main loop now computes should_poll using cycle-clock delta from busy_poll_start_cycle() rather than GetCurrentBusyCycles().
mpmc cell cache-line alignment & SC dequeue
base/mpmc_bounded_queue.h, base/mpmc_bounded_queue_test.cc
Add try_dequeue_sc single-consumer dequeue, adjust capacity formatting, align internal cell_t to cache-line alignment, and add a unit test validating SC dequeue.
FiberQueue exception propagation
util/fibers/fiberqueue_threadpool.cc
Removed local try/catch(std::exception&) around dequeued callback; exceptions now propagate from FiberQueue::Run().
Tasklet type & function2 include diagnostics
util/fibers/fiberqueue_threadpool.h
Add Fu2Fun alias and Tasklet type built from fu2::function_base; wrap base/function2.hpp include with GCC diagnostic push/pop ignoring -Waddress; change task queue element type to Tasklet.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • dranikpg

Poem

I nudge the loop and skip a spin,
I reset my clock and tuck time in.
When idle hops ask me to stay,
I run or pause, then find my way.
A rabbit proactor—soft and keen. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: replacing the fixed spin-loop threshold with a busy-poll cycle limit in the Epoll implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch epoll-busy-poll-limit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the busy polling mechanism in EpollProactor::MainLoop by replacing the loop-count-based spinning with a cycle-based busy poll limit. It introduces ResetBusyPoll() calls across various execution paths to refresh the polling window. The review feedback highlights a critical discrepancy where GetCurrentBusyCycles() does not utilize the cycle updated by ResetBusyPoll(), rendering the reset ineffective. Additionally, it is recommended to reset the busy poll window when active idle tasks are processed to prevent premature blocking.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread util/fibers/epoll_proactor.cc Outdated
// 3. Task queue is empty otherwise we should spin more to unload it.
if (task_queue_exhausted && !scheduler->HasAnyReady() && spin_loops >= kMaxSpinLimit) {
spin_loops = 0;
bool should_poll = GetCurrentBusyCycles() < busy_poll_cycle_limit_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There is a discrepancy between GetCurrentBusyCycles() and ResetBusyPoll().

Currently, ProactorBase::GetCurrentBusyCycles() is implemented as:

uint64_t ProactorBase::GetCurrentBusyCycles() const {
  return base::CycleClock::Now() - io_wait_end_cycle_;
}

However, ResetBusyPoll() only updates busy_poll_start_cycle_:

void ProactorBase::ResetBusyPoll() {
  busy_poll_start_cycle_ = base::CycleClock::Now();
}

Because GetCurrentBusyCycles() relies on io_wait_end_cycle_ (which is only updated in IdleEnd when a blocking wait ends), calling ResetBusyPoll() has no effect on the value returned by GetCurrentBusyCycles(). As a result, the busy-poll window is never actually reset after real CPU or I/O work, and should_poll will prematurely evaluate to false once the cycle limit since the last idle period is exceeded.

To fix this, you can either:

  1. Update ProactorBase::GetCurrentBusyCycles() in proactor_base.cc to use busy_poll_start_cycle_ instead of io_wait_end_cycle_.
  2. Or, directly calculate the busy cycles using busy_poll_start_cycle_ in EpollProactor as suggested below.
    bool should_poll = (base::CycleClock::Now() - busy_poll_start_cycle_) < busy_poll_cycle_limit_;

Comment on lines +320 to 322
if (spin_on_idle_tasks) {
continue;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If spin_on_idle_tasks is true, it indicates that there are active idle tasks requiring the proactor to continue spinning. However, if we do not call ResetBusyPoll() here, the busy poll cycle limit (should_poll) may be exceeded in the next iteration, causing the proactor to prematurely enter the wait section and block. We should reset the busy poll window before continuing.

    if (spin_on_idle_tasks) {
      ResetBusyPoll();
      continue;
    }

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
util/fibers/epoll_proactor.cc (1)

281-298: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Move ResetBusyPoll() after completion dispatch.

Resetting here starts the next busy-poll window before DispatchCompletions() runs, so callback CPU time gets charged against the idle-spin budget. That makes epoll age out of busy-polling earlier than the uring loop this refactor is trying to match.

♻️ Suggested change
     if (cqe_count) {
       ++stats_.completions_fetches;
-      ResetBusyPoll();

       while (true) {
         VPRO(2) << "Fetched " << cqe_count << " cqes";
         DispatchCompletions(&ev_batch, cqe_count);

         if (cqe_count < kEvBatchSize) {
           break;
         }
         epoll_res = EpollWait(epoll_fd_, &ev_batch, 0);
         if (epoll_res < 0) {
           break;
         }
         cqe_count = epoll_res;
         ++stats_.completions_fetches;
       };
+      ResetBusyPoll();
     }

Based on the PR objective to reset busy-poll after real work and the uring reference in util/fibers/uring_proactor.cc:881-940, the reset point should follow completion processing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/fibers/epoll_proactor.cc` around lines 281 - 298, Move the
ResetBusyPoll() call so it runs after processing completions: currently
ResetBusyPoll() is invoked immediately after detecting cqe_count in the epoll
path (inside the if (cqe_count) block) which starts the next busy-poll window
before DispatchCompletions(&ev_batch, cqe_count) executes; update the block
around DispatchCompletions, EpollWait(epoll_fd_, &ev_batch, 0), cqe_count and
stats_.completions_fetches so ResetBusyPoll() is called only after
DispatchCompletions returns (and before looping to the next EpollWait), matching
the uring_proactor behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 281-298: Move the ResetBusyPoll() call so it runs after processing
completions: currently ResetBusyPoll() is invoked immediately after detecting
cqe_count in the epoll path (inside the if (cqe_count) block) which starts the
next busy-poll window before DispatchCompletions(&ev_batch, cqe_count) executes;
update the block around DispatchCompletions, EpollWait(epoll_fd_, &ev_batch, 0),
cqe_count and stats_.completions_fetches so ResetBusyPoll() is called only after
DispatchCompletions returns (and before looping to the next EpollWait), matching
the uring_proactor behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7ad7119b-2bf0-4cf6-ac55-5d8e1a1c7dd1

📥 Commits

Reviewing files that changed from the base of the PR and between dc37855 and c15c786.

📒 Files selected for processing (1)
  • util/fibers/epoll_proactor.cc

@romange
romange force-pushed the epoll-busy-poll-limit branch from c15c786 to 08846a1 Compare June 7, 2026 06:06

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
util/fibers/epoll_proactor.cc (1)

202-220: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Double-counting of stats_.num_task_runs.

Line 202 increments stats_.num_task_runs for each task inside the loop, but line 218 adds cnt again after the loop exits. This causes each task run to be counted twice.

🐛 Proposed fix

Remove the duplicate addition at line 218:

       } while (task_queue_.try_dequeue(task));

-      stats_.num_task_runs += cnt;
       DVLOG(2) << "Tasks runs " << stats_.num_task_runs;
       ResetBusyPoll();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/fibers/epoll_proactor.cc` around lines 202 - 220, The loop increments
stats_.num_task_runs for each task and also accumulates cnt, then adds cnt again
after the loop, causing double-counting; fix by removing the post-loop addition
of cnt (the line updating stats_.num_task_runs += cnt) so that only the per-task
increments inside the task handling loop (where stats_.num_task_runs is
incremented) account for runs; locate the block around task_queue_.try_dequeue,
the local cnt variable, and the post-loop stats_.num_task_runs += cnt and delete
that duplicate update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@util/fibers/epoll_proactor.cc`:
- Around line 202-220: The loop increments stats_.num_task_runs for each task
and also accumulates cnt, then adds cnt again after the loop, causing
double-counting; fix by removing the post-loop addition of cnt (the line
updating stats_.num_task_runs += cnt) so that only the per-task increments
inside the task handling loop (where stats_.num_task_runs is incremented)
account for runs; locate the block around task_queue_.try_dequeue, the local cnt
variable, and the post-loop stats_.num_task_runs += cnt and delete that
duplicate update.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: dc00b247-0bb8-48ec-8034-4a54b22c6e9b

📥 Commits

Reviewing files that changed from the base of the PR and between c15c786 and 08846a1.

📒 Files selected for processing (4)
  • util/fibers/epoll_proactor.cc
  • util/fibers/proactor_base.cc
  • util/fibers/proactor_base.h
  • util/fibers/uring_proactor.cc

@romange
romange force-pushed the epoll-busy-poll-limit branch from 08846a1 to f9923ae Compare June 7, 2026 07:37

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
util/fibers/epoll_proactor.cc (1)

220-220: ⚡ Quick win

Recompute should_poll from current state at the idle tail.

Line 324 consumes the value captured at Line 237 even though ResetBusyPoll() can advance busy_poll_start_cycle_ earlier in the same iteration. After L2/background/completion work, the tail can still see a stale false and skip the short spin that the uring loop bases on current state.

Suggested change
-    bool should_poll = (base::CycleClock::Now() - busy_poll_start_cycle()) < busy_poll_cycle_limit_;
-    if (task_queue_exhausted && !scheduler->HasAnyReady() && !should_poll) {
+    auto should_poll = [this] {
+      return (base::CycleClock::Now() - busy_poll_start_cycle()) < busy_poll_cycle_limit_;
+    };
+    if (task_queue_exhausted && !scheduler->HasAnyReady() && !should_poll()) {
       if (tq_seq_.compare_exchange_weak(tq_seq, WAIT_SECTION_STATE, memory_order_acquire)) {
         // We check stop condition when all the pending events were processed.
         // It's up to the app-user to make sure that the incoming flow of events is stopped before
         // stopping EpollProactor.
@@
-    if (should_poll) {
+    if (should_poll()) {
       Pause(3);
     }

Also applies to: 237-238, 283-283, 301-325

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/fibers/epoll_proactor.cc` at line 220, The code captures should_poll
early but calls ResetBusyPoll() which can change busy_poll_start_cycle_, leading
to a stale false; to fix, recompute should_poll from the current state after
every ResetBusyPoll() (e.g., call the same predicate/compute that reads
busy_poll_start_cycle_ or use ShouldBusyPoll()) before the idle-tail short-spin
decision so the uring loop uses up-to-date busy-poll timing; apply the same
recomputation at the other noted sites (around uses at the locations referencing
should_poll and busy_poll_start_cycle_, including the blocks around
ResetBusyPoll()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@util/fibers/fiberqueue_threadpool.cc`:
- Line 43: The removal of the try/catch around func() lets exceptions escape and
will terminate the worker thread; restore exception safety by reintroducing a
try/catch in the same scope where func() is invoked (the call site inside
Run()/WorkerFunction()), catch std::exception& and a catch(...) fallback, log
the error (including exception.what() when available) and prevent the exception
from propagating to the pthread entry point so the thread doesn't call
std::terminate(); alternatively, if you intend to enforce non-throwing
callbacks, wrap registered callbacks with a noexcept wrapper at registration
time and document the breaking change.

In `@util/fibers/fiberqueue_threadpool.h`:
- Around line 28-35: Tasklet is dead code — either remove it or actually use it:
either delete the Tasklet struct and its Fu2Fun typedef to eliminate unused
code, or replace the existing std::function alias CbFunc/FuncQ and the consumer
variable CbFunc func in FiberQueue/FiberQueue::consumer loop with Tasklet so the
queue stores fu2::function-based callables; update all references from
std::function<void()> to Tasklet (or remove Tasklet entirely) and adjust any
construction/assignment sites to use Fu2Fun semantics accordingly.

---

Nitpick comments:
In `@util/fibers/epoll_proactor.cc`:
- Line 220: The code captures should_poll early but calls ResetBusyPoll() which
can change busy_poll_start_cycle_, leading to a stale false; to fix, recompute
should_poll from the current state after every ResetBusyPoll() (e.g., call the
same predicate/compute that reads busy_poll_start_cycle_ or use
ShouldBusyPoll()) before the idle-tail short-spin decision so the uring loop
uses up-to-date busy-poll timing; apply the same recomputation at the other
noted sites (around uses at the locations referencing should_poll and
busy_poll_start_cycle_, including the blocks around ResetBusyPoll()).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eafcc436-a6a0-4405-bbb5-c7891ccdb95d

📥 Commits

Reviewing files that changed from the base of the PR and between 08846a1 and f9923ae.

📒 Files selected for processing (7)
  • base/mpmc_bounded_queue.h
  • util/fibers/epoll_proactor.cc
  • util/fibers/fiberqueue_threadpool.cc
  • util/fibers/fiberqueue_threadpool.h
  • util/fibers/proactor_base.cc
  • util/fibers/proactor_base.h
  • util/fibers/uring_proactor.cc
✅ Files skipped from review due to trivial changes (1)
  • util/fibers/proactor_base.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • util/fibers/uring_proactor.cc
  • util/fibers/proactor_base.cc

// std::exception_ptr p = std::current_exception();
LOG(FATAL) << "Exception " << e.what();
}
func();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Unhandled exceptions will now terminate the worker thread.

Removing the try/catch wrapper allows exceptions from func() to propagate through Run()WorkerFunction() → the pthread thread entry point, which will call std::terminate() and crash the worker thread. While the documented contract (see util/fibers/uring_socket.h:48) states callbacks "must not throw," removing this safety net without adding termination-proof guards or process-level exception handling is risky and violates availability guarantees.

🛡️ Proposed fix to restore exception safety
-    func();
+    try {
+      func();
+    } catch (const std::exception& e) {
+      LOG(FATAL) << "Exception in FiberQueue callback: " << e.what();
+    } catch (...) {
+      LOG(FATAL) << "Unknown exception in FiberQueue callback";
+    }

Alternatively, if the goal is to enforce the "must not throw" contract, document this breaking change and ensure all callback registration sites add noexcept wrappers.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/fibers/fiberqueue_threadpool.cc` at line 43, The removal of the
try/catch around func() lets exceptions escape and will terminate the worker
thread; restore exception safety by reintroducing a try/catch in the same scope
where func() is invoked (the call site inside Run()/WorkerFunction()), catch
std::exception& and a catch(...) fallback, log the error (including
exception.what() when available) and prevent the exception from propagating to
the pthread entry point so the thread doesn't call std::terminate();
alternatively, if you intend to enforce non-throwing callbacks, wrap registered
callbacks with a noexcept wrapper at registration time and document the breaking
change.

Comment thread util/fibers/fiberqueue_threadpool.h Outdated
@romange
romange force-pushed the epoll-busy-poll-limit branch 2 times, most recently from 5e35a21 to d1a4a4b Compare June 7, 2026 07:55

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
base/mpmc_bounded_queue_test.cc (1)

116-132: ⚡ Quick win

Exercise try_dequeue_sc with a non-trivial payload.

This only proves FIFO on int. try_dequeue_sc duplicates the manual move/destroy path from try_dequeue, so please add a Moveable or A case here to catch object-lifetime regressions on the new API.

Suggested coverage shape
+TEST_F(MPMCTest, SingleConsumerDequeueMoveable) {
+  mpmc_bounded_queue<Moveable> q(2);
+  EXPECT_TRUE(q.try_enqueue(Moveable{}));
+
+  {
+    Moveable dest;
+    EXPECT_TRUE(q.try_dequeue_sc(dest));
+    EXPECT_EQ(1, Moveable::ref);
+  }
+
+  EXPECT_EQ(0, Moveable::ref);
+}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@base/mpmc_bounded_queue_test.cc` around lines 116 - 132, The test
SingleConsumerDequeue only exercises try_dequeue_sc with ints; update it to use
a non-trivial movable type (e.g., Moveable or A) so the single-consumer path
that does manual move/destroy is exercised; specifically, change the queue type
to mpmc_bounded_queue<Moveable> (or A), enqueue a few Moveable instances
(verifying their payloads or move count), then dequeue with try_dequeue_sc into
a Moveable variable and assert the moved values/state and that no extra
copies/destructors occur as expected; keep the same enqueue/dequeue ordering and
failure assertion at the end to preserve FIFO and empty-queue behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@base/mpmc_bounded_queue_test.cc`:
- Around line 116-132: The test SingleConsumerDequeue only exercises
try_dequeue_sc with ints; update it to use a non-trivial movable type (e.g.,
Moveable or A) so the single-consumer path that does manual move/destroy is
exercised; specifically, change the queue type to mpmc_bounded_queue<Moveable>
(or A), enqueue a few Moveable instances (verifying their payloads or move
count), then dequeue with try_dequeue_sc into a Moveable variable and assert the
moved values/state and that no extra copies/destructors occur as expected; keep
the same enqueue/dequeue ordering and failure assertion at the end to preserve
FIFO and empty-queue behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c83b652b-8d4b-4f72-af19-e8f50b27b41e

📥 Commits

Reviewing files that changed from the base of the PR and between f9923ae and 5e35a21.

📒 Files selected for processing (8)
  • base/mpmc_bounded_queue.h
  • base/mpmc_bounded_queue_test.cc
  • util/fibers/epoll_proactor.cc
  • util/fibers/fiberqueue_threadpool.cc
  • util/fibers/fiberqueue_threadpool.h
  • util/fibers/proactor_base.cc
  • util/fibers/proactor_base.h
  • util/fibers/uring_proactor.cc
✅ Files skipped from review due to trivial changes (1)
  • util/fibers/proactor_base.h
🚧 Files skipped from review as they are similar to previous changes (5)
  • util/fibers/fiberqueue_threadpool.cc
  • util/fibers/uring_proactor.cc
  • util/fibers/fiberqueue_threadpool.h
  • util/fibers/proactor_base.cc
  • util/fibers/epoll_proactor.cc

@romange
romange force-pushed the epoll-busy-poll-limit branch from d1a4a4b to 26d25d9 Compare June 7, 2026 08:00

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
util/fibers/fiberqueue_threadpool.h (1)

28-35: 💤 Low value

Consider adding static_assert for size verification.

ProactorBase includes static_assert(sizeof(Tasklet) == 32, "") after its Tasklet definition to catch unintended size changes. Given the larger capacity_fixed<32, 8> here, FiberQueue's Tasklet will be larger (likely 48 bytes). Adding a similar assertion would maintain consistency with the ProactorBase pattern and guard against unexpected size regressions.

♻️ Suggested addition
 struct Tasklet : public Fu2Fun {
   using Fu2Fun::Fu2Fun;
   using Fu2Fun::operator=;
 };
+static_assert(sizeof(Tasklet) == 48, "");

Adjust the expected size based on actual measured value.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/fibers/fiberqueue_threadpool.h` around lines 28 - 35, The Tasklet alias
(struct Tasklet : public Fu2Fun) uses fu2::capacity_fixed<32, 8> so its sizeof
has grown compared to ProactorBase; add a static_assert after the Tasklet
definition to lock the expected size (e.g. static_assert(sizeof(Tasklet) == 48,
"");) — reference the Tasklet type and Fu2Fun/capacity_fixed<32, 8> to locate
the spot and adjust the numeric expectation to the measured sizeof(Tasklet) on
your platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@util/fibers/fiberqueue_threadpool.h`:
- Around line 28-35: The Tasklet alias (struct Tasklet : public Fu2Fun) uses
fu2::capacity_fixed<32, 8> so its sizeof has grown compared to ProactorBase; add
a static_assert after the Tasklet definition to lock the expected size (e.g.
static_assert(sizeof(Tasklet) == 48, "");) — reference the Tasklet type and
Fu2Fun/capacity_fixed<32, 8> to locate the spot and adjust the numeric
expectation to the measured sizeof(Tasklet) on your platform.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0b00ee6b-10fc-4024-8908-cfdae2b80f8e

📥 Commits

Reviewing files that changed from the base of the PR and between 5e35a21 and d1a4a4b.

📒 Files selected for processing (8)
  • base/mpmc_bounded_queue.h
  • base/mpmc_bounded_queue_test.cc
  • util/fibers/epoll_proactor.cc
  • util/fibers/fiberqueue_threadpool.cc
  • util/fibers/fiberqueue_threadpool.h
  • util/fibers/proactor_base.cc
  • util/fibers/proactor_base.h
  • util/fibers/uring_proactor.cc
🚧 Files skipped from review as they are similar to previous changes (7)
  • util/fibers/uring_proactor.cc
  • base/mpmc_bounded_queue_test.cc
  • util/fibers/proactor_base.h
  • util/fibers/fiberqueue_threadpool.cc
  • base/mpmc_bounded_queue.h
  • util/fibers/proactor_base.cc
  • util/fibers/epoll_proactor.cc

@romange
romange force-pushed the epoll-busy-poll-limit branch 2 times, most recently from 64a1a21 to 4d6c0bc Compare June 11, 2026 13:11
- Replace the fixed spin-loop threshold with the shared busy-poll cycle budget in EpollProactor.

- Reset the busy-poll window after real CPU or I/O work, matching the UringProactor loop behavior.

- Keep zero-timeout epoll probes from resetting idle accounting so they count against the spill budget.

Test: built fibers2
Signed-off-by: Roman Gershman <romange@gmail.com>
@romange
romange force-pushed the epoll-busy-poll-limit branch from 4d6c0bc to e69f40a Compare June 11, 2026 13:34
romange added a commit that referenced this pull request Jun 14, 2026
Address PR review and pull the callback-type improvement from #595.

Shutdown race (lost callback + deadlock): a blocking Add() that has claimed a slot via
claim_slot() but not yet committed it is invisible to try_dequeue_sc, so Run() could see an
empty-looking queue, observe is_closed_, and exit while the producer was still parked between
claim and commit -- stranding the callback and deadlocking any Await waiting on it. Add
mpmc_bounded_queue::has_inflight_slots() and gate Run()'s exit on
is_closed_ && !queue_.has_inflight_slots(), keeping the consumer parked until the producer
commits and notifies pull_ec_.

claim_slot() now uses memory_order_relaxed instead of acq_rel: enqueue_pos_ only hands out
unique tickets and carries no data; publication is done by commit_slot's release store and
observed by slot_ready/try_dequeue acquire loads. Matches try_enqueue.

Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

Rejected the bots' FiberActive()->Yield() null-check: FiberActive() lazily constructs the
thread-local main-context stub and is never null.

Adds MPMCTest.HasInflightSlots.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
romange added a commit that referenced this pull request Jul 5, 2026
Address PR review and pull the callback-type improvement from #595.

Shutdown race (lost callback + deadlock): a blocking Add() that has claimed a slot via
claim_slot() but not yet committed it is invisible to try_dequeue_sc, so Run() could see an
empty-looking queue, observe is_closed_, and exit while the producer was still parked between
claim and commit -- stranding the callback and deadlocking any Await waiting on it. Add
mpmc_bounded_queue::has_inflight_slots() and gate Run()'s exit on
is_closed_ && !queue_.has_inflight_slots(), keeping the consumer parked until the producer
commits and notifies pull_ec_.

claim_slot() now uses memory_order_relaxed instead of acq_rel: enqueue_pos_ only hands out
unique tickets and carries no data; publication is done by commit_slot's release store and
observed by slot_ready/try_dequeue acquire loads. Matches try_enqueue.

Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

Rejected the bots' FiberActive()->Yield() null-check: FiberActive() lazily constructs the
thread-local main-context stub and is never null.

Adds MPMCTest.HasInflightSlots.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
romange added a commit that referenced this pull request Jul 5, 2026
1. Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

2. FiberQueue: put pull_ec_ and push_ec_ on separate cache lines
pull_ec_ is written by every producer (notify() after each commit); push_ec_ is written by the
consumer (notifyAll() per drained batch) and by blocked producers. Adjacent in memory they share a
cache line and ping-pong between producer- and consumer-side wakeup traffic. alignas(64) each onto
its own line.

3. Add blocked_submitters_ metric

Signed-off-by: Roman Gershman <romange@gmail.com>
romange added a commit that referenced this pull request Jul 5, 2026
1. Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

2. FiberQueue: put pull_ec_ and push_ec_ on separate cache lines
pull_ec_ is written by every producer (notify() after each commit); push_ec_ is written by the
consumer (notifyAll() per drained batch) and by blocked producers. Adjacent in memory they share a
cache line and ping-pong between producer- and consumer-side wakeup traffic. alignas(64) each onto
its own line.

3. Add blocked_submitters_ metric

Signed-off-by: Roman Gershman <romange@gmail.com>
romange added a commit that referenced this pull request Jul 5, 2026
1. Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

2. FiberQueue: put pull_ec_ and push_ec_ on separate cache lines
pull_ec_ is written by every producer (notify() after each commit); push_ec_ is written by the
consumer (notifyAll() per drained batch) and by blocked producers. Adjacent in memory they share a
cache line and ping-pong between producer- and consumer-side wakeup traffic. alignas(64) each onto
its own line.

3. Add blocked_submitters_ metric

Signed-off-by: Roman Gershman <romange@gmail.com>
romange added a commit that referenced this pull request Jul 5, 2026
1. Replace the std::function<void()> callback (CbFunc) with Tasklet, a fixed-capacity
fu2::function_base (capacity_fixed<32,8>, non-copyable, non-throwing). Callbacks up to 32 bytes
are stored inline with no heap allocation. Ported from #595.

2. FiberQueue: put pull_ec_ and push_ec_ on separate cache lines
pull_ec_ is written by every producer (notify() after each commit); push_ec_ is written by the
consumer (notifyAll() per drained batch) and by blocked producers. Adjacent in memory they share a
cache line and ping-pong between producer- and consumer-side wakeup traffic. alignas(64) each onto
its own line.

3. Add blocked_submitters_ metric

Signed-off-by: Roman Gershman <romange@gmail.com>
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