Repository navigation
perf(processor): run a one-thread task on the calling thread - #1297
Conversation
|
Running a one-thread task on the calling thread changes one Python driver test: 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 |
adsharma
left a comment
There was a problem hiding this comment.
Requesting changes on the concurrency handling — the optimization direction is sound, but three items need addressing before merge:
- Verify
numBusyThreadsaccounting is locked. In the shownrunWorkerThreadhunk the decrement appears afterlck.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 holdstaskSchedulerMtx. thread_local busySlotOwnerlifetime. 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 makesbusySlotOwner == thistrue spuriously — nested tasks then bypass the slot cap on an unrelated scheduler. Clear it on worker/thread exit.- GDS
launchNewWorkerThreadslot claim. The new thread 'reuses the caller's slot' only as a flag — it never incrementsnumBusyThreads, and nested tasks on it take thebusySlotOwner == thisfast 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.
|
Thanks. Follow-up commit db1fa59 is on top.
Left-spine walk. A pipeline is the child-0 chain from sink to source, the walk Overloads. Collapsed to two with Full ctest 3106/3106; scheduler tests pass 100 repeats. |
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>
775e7b4 to
bea4a7a
Compare
This work was produced with the help of language models.
Fixes #1296. Part of #1290.
scheduleTaskAndWaitOrErrorpushed every task to the queue and woke the whole worker pool, even when only one thread could register. A one-rowREAD_FTABLEsource (RETURN 1and other constant queries) was also marked parallel, so every worker cloned the pipeline before finding no morsel: 0.94 ms to executeRETURN 1on 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
numWorkerThreadsslots, somax_num_threadsstill 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, aREAD_FTABLEscan that fits in one morsel is lowered to one thread. Extension ABI: members are appended, butscheduleTaskAndWaitOrErrornow takes a defaultedbeforeRun, 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 1in a fresh in-memory database, query-summary execute time, median of 8 processes; warm is a C API loop of 20000 queries.Nine LDBC SF1 queries at one thread (joins, aggregates, sort, a 100K-row collect): identical rows, instructions within ±0.2%.
Tests. Six
TaskSchedulerTestcases: 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 forlaunchNewWorkerThreadtakes 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 inApiTest.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.