Skip to content

perf(processor): run a one-thread task on the calling thread - #1297

Merged
adsharma merged 2 commits into
LadybugDB:mainfrom
zahariash:perf/inline-one-thread-tasks
Oct 21, 2026
Merged

adsharma merged 2 commits into
LadybugDB:mainfrom
zahariash:perf/inline-one-thread-tasks

Conversation

@zahariash

@zahariash zahariash commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

This work was produced with the help of language models.

Fixes #1296. Part of #1290.

scheduleTaskAndWaitOrError pushed every task to the queue and woke the whole worker pool, even when only one thread could register. A one-row READ_FTABLE source (RETURN 1 and other constant queries) was also marked parallel, so every worker cloned the pipeline before finding no morsel: 0.94 ms to execute RETURN 1 on a fresh database, 157 us per query warm.

Fix. A task limited to one thread runs on the calling thread and the pool stays asleep. Workers and inline callers share the database's numWorkerThreads slots, so max_num_threads still bounds the threads running queries across connections: 8 connections each running a heavy single-threaded query on a 2-thread database use 2.73 cores, as on main. A caller that finds no free slot queues as before; a scope guard returns the slot, also when the task throws, and wakes one worker while tasks are queued. Queries with a timeout keep the queue path, since the waiting caller enforces the timeout, as do callers that launch their own worker thread (GDS, the LLM extension); that thread takes over the caller's slot if the caller holds one. After a task's dependencies finish, a READ_FTABLE scan that fits in one morsel is lowered to one thread. Extension ABI: members are appended, but scheduleTaskAndWaitOrError now takes a defaulted beforeRun, so the symbol the algo and llm extensions link against changes: fine for 0.22.0, where extensions are rebuilt; a 0.21.x backport would need the forwarding overloads back.

Performance. RETURN 1 in a fresh in-memory database, query-summary execute time, median of 8 processes; warm is a C API loop of 20000 queries.

main this PR
1st query 0.94 ms 0.54 ms
2nd query 0.30 ms 0.04 ms
3rd query 0.20 ms 0.02 ms
warm 157 us 59 us

Nine LDBC SF1 queries at one thread (joins, aggregates, sort, a 100K-row collect): identical rows, instructions within ±0.2%.

Tests. Six TaskSchedulerTest cases: a one-thread task runs on the calling thread; 8 callers on a 2-thread database peak at 2 threads (8 without the slot accounting); tasks queued while every slot is held inline run side by side once the callers return (fails in 20 of 20 runs if a slot release wakes a worker only when the pool was full); a nested one-thread task under a 1-thread limit reuses the caller's slot (hangs otherwise); a thread launched for launchNewWorkerThread takes over its caller's slot (hangs otherwise) and holds none when the caller has none (fails if it always claims one). Full ctest on this branch: 3106/3106. One earlier run had a single failed reader query in ApiTest.ConcurrentCheckpointRead1160; it did not come back in the re-run or in 450 stress runs of that test, and 450 with main's scheduler did not show it either.

@zahariash

Copy link
Copy Markdown
Contributor Author

Running a one-thread task on the calling thread changes one Python driver test: test_close_with_query_in_flight[close_from_a_udf] fails, 3 of 3 runs on this branch and none on main. The UDF in that test runs in a one-thread task, so it now runs on the thread that issued the query, and ladybug-python's close() sees that connection's own call on that thread and refuses with "close() cannot be called from a call on the same connection" instead of the UDF message. The close is still refused, and the CI jobs here don't run the driver suite. The same mismatch already happens on main for a UDF folded while binding; LadybugDB/ladybug-python#71 fixes both, and with it the driver suite against this branch (without the torch-geometric and JSON-extension tests) matches main.

I also stress-tested this PR together with #1286, #1300, #1294 and a local parser change: full ctest 3109/3109, our workload simulator (300 workloads), 8 threads of checked queries with random timeouts, interrupts and a writer at 1, 4 and 8 engine threads (about 381k queries, no errors or hangs), and LDBC SF1, where all 44 queries return the same results as main.

@adsharma adsharma 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.

Requesting changes on the concurrency handling — the optimization direction is sound, but three items need addressing before merge:

  1. Verify numBusyThreads accounting is locked. In the shown runWorkerThread hunk the decrement appears after lck.unlock(). If that is the real code it is a data race against the locked increments/checks — slot over/under-count leading to thread-limit bypass or worker starvation. Please confirm the decrement holds taskSchedulerMtx.
  2. thread_local busySlotOwner lifetime. Workers set it on entry and never clear it (same for GDS/LLM-spawned threads). After a Database/scheduler is destroyed, a pooled OS thread keeps a dangling pointer, and a new database allocated at the same address makes busySlotOwner == this true spuriously — nested tasks then bypass the slot cap on an unrelated scheduler. Clear it on worker/thread exit.
  3. GDS launchNewWorkerThread slot claim. The new thread 'reuses the caller's slot' only as a flag — it never increments numBusyThreads, and nested tasks on it take the busySlotOwner == this fast path with no counting, so that subtree is uncapped. Not a regression (the GDS thread already exists outside the pool), but the comment overclaims; please clarify or count it.

Also worth confirming narrowToSingleMorselSource's left-spine walk is valid for multi-child operators (joins/unions), and considering collapsing the four scheduleTaskAndWaitOrError overloads with a defaulted beforeRun = nullptr.

@zahariash

Copy link
Copy Markdown
Contributor Author

Thanks. Follow-up commit db1fa59 is on top.

  1. numBusyThreads locking. Every access holds taskSchedulerMtx. The hunk starts mid-loop: lck.lock() at the loop top (task_scheduler.cpp:194) covers the decrement (:202), cv.wait returns locked for the increment (:211), and lck.unlock() follows at :213. runOnCallingThread increments under the mutex (:145-149), SlotGuard decrements under it (:157-158).

  2. busySlotOwner lifetime. No pooled thread can keep the pointer. Only workers and the launchNewWorkerThread thread keep it set until exit; both are joined before the scheduler is destroyed (workers in ~TaskScheduler, the new thread before scheduleTaskAndWaitOrError returns), and a thread_local ends with its thread. Callers set it only inside runOnCallingThread; SlotGuard restores the previous value on return or throw. The comment now says so.

  3. launchNewWorkerThread slot. Agreed, the comment overclaimed. The new thread now inherits the caller's busySlotOwner. A GDS caller runs inside a task and blocks for the whole run, so the new thread works in its slot; a slotless caller (e.g. AI_EXTRACT folded at bind time) passes none, and nested tasks on the new thread are counted. Counting the new thread too would count the blocked caller twice and leave GDS one worker short of main. Tests: NewWorkerThreadTakesOverCallerSlot (hangs without the takeover) and NewWorkerThreadOfSlotlessCallerHoldsNoSlot (fails with the previous busySlotOwner = this). GDS comment updated.

Left-spine walk. A pipeline is the child-0 chain from sink to source, the walk initTask (which sets the thread limit) and initLocalState already use; decomposePlanIntoTask runs every sink below as a child task. The other children of HashJoinProbe, Intersect, CrossProduct and PathPropertyProbe are such sinks; UnionAllScan is itself the source. So the walk ends at the pipeline's morsel source. Comment added.

Overloads. Collapsed to two with beforeRun = nullptr. The algo and llm extensions call the three-argument form, so the symbol they link against changes: fine for 0.22.0 (extensions are rebuilt); a 0.21.x backport would need the forwarding overloads back.

Full ctest 3106/3106; scheduler tests pass 100 repeats.

zahariash and others added 2 commits October 20, 2026 20:36
scheduleTaskAndWaitOrError pushed every task to the queue and woke the
whole worker pool, even when only one thread could register. A one-row
READ_FTABLE (RETURN 1 and other constant queries) was also marked
parallel, so every worker cloned the pipeline before finding no morsel.

A task limited to one thread now runs on the caller. Workers and inline
callers share numWorkerThreads slots under the scheduler mutex, so
max_num_threads still bounds the threads running queries across
connections; a caller that finds no free slot uses the queue, and a
thread that already holds a slot reuses it. Queries with a timeout and
callers that pass launchNewWorkerThread keep the queue path. After a
task's children finish, a READ_FTABLE source that fits in one morsel is
lowered to one thread.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A thread started for launchNewWorkerThread marked itself as holding a
scheduler slot even when its caller held none, so nested one-thread
tasks on it skipped the slot count. It now inherits the caller's slot
owner: a GDS call runs inside a task, so its blocked caller's slot
passes to the new thread; a caller without a slot passes none.

Collapse the four scheduleTaskAndWaitOrError overloads into two with a
defaulted beforeRun, note why busySlotOwner never outlives its
scheduler and that a pipeline's source is at the end of its child-0
chain, and update the GDS comment that assumed a worker caller.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adsharma
adsharma force-pushed the perf/inline-one-thread-tasks branch from 775e7b4 to bea4a7a Compare October 21, 2026 03:36
@adsharma
adsharma merged commit 60443ff into LadybugDB:main Oct 21, 2026
4 checks passed
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.

A one-thread task wakes the whole worker pool, 0.9 ms executing a constant query

2 participants