diff --git a/.cspell/custom-dictionary-workspace.txt b/.cspell/custom-dictionary-workspace.txt index 6ef384d4b..d5ff0f1ef 100644 --- a/.cspell/custom-dictionary-workspace.txt +++ b/.cspell/custom-dictionary-workspace.txt @@ -25,6 +25,7 @@ armhf armv ASHP asyncio +atfork authoriser autodocstring autoflake @@ -380,6 +381,7 @@ preseeded prevs protobuf psum +pthread pvbat pvenergytotal pvlib @@ -537,10 +539,14 @@ venv Victron visibilitychange waf +waitpid Wallbox weblink welink Werror +WEXITSTATUS +WIFEXITED +WNOHANG workmode writeonly wrongsha diff --git a/apps/predbat/plan.py b/apps/predbat/plan.py index f3efa39f1..2c8c40ab6 100644 --- a/apps/predbat/plan.py +++ b/apps/predbat/plan.py @@ -35,6 +35,26 @@ PLAN_PASS_WINDOW_BUDGET = 8 +def resolve_batch_threads(threads, cpu_count_value): + """Map the threads setting onto how many kernel lanes one batch may use. + + 'auto' takes the core count and is deliberately not capped. On a fast machine the curve is very + flat and peaks slightly below the core count - measured on the 20-scenario benchmark, best of 3: + serial 26.33s, 4 threads 24.89s, 6 threads 24.71s, 8 threads 24.91s, 16 threads 25.04s - so a cap + looks attractive. But re-running with each job made eight times dearer, which is how a machine + where the kernel dominates behaves, the curve stops turning over entirely: 48.92s serial, 32.03s + at 4, 29.98s at 6, 29.85s at 8, 28.94s at 16. + + That makes the risk asymmetric. Capping at 4 costs 0.7% on the fast machine but 10.7% on the + kernel-heavy one, while not capping costs 1.3% at worst. The worst case for a low cap is far + worse than the worst case for none, so 'auto' is left alone and anyone who wants fewer lanes sets + threads: explicitly. + """ + if threads == "auto": + return max(cpu_count_value, 1) + return max(int(threads), 1) + + def slots_around(target_slots, slot_lengths): """ Return a list of slot lengths around the target slots @@ -1306,11 +1326,7 @@ def calculate_plan(self, recompute=True, debug_mode=False, publish=True): # The kernel spreads one batched fan-out across threads with the GIL released for the whole # call, so these are real cores - unlike a Python ThreadPool, which peaked at 1.15x on two # threads and then degraded below serial (perf/threadpool-prototype). - threads = self.get_arg("threads", "auto") - if threads == "auto": - self.prediction.batch_threads = cpu_count() - else: - self.prediction.batch_threads = max(int(threads), 1) + self.prediction.batch_threads = resolve_batch_threads(self.get_arg("threads", "auto"), cpu_count()) self.log("Prediction batch using {} kernel thread(s)".format(self.prediction.batch_threads)) kernel_message, kernel_is_warning = kernel_status_summary(self.prediction) self.log("{}Prediction kernel: {}".format("Warn: " if kernel_is_warning else "", kernel_message)) diff --git a/apps/predbat/prediction_kernel.cpp b/apps/predbat/prediction_kernel.cpp index 6b1d2bde6..0be4dd4f8 100644 --- a/apps/predbat/prediction_kernel.cpp +++ b/apps/predbat/prediction_kernel.cpp @@ -16,6 +16,8 @@ // Build: bash apps/predbat/build_kernel.sh (g++/clang, C++17, no dependencies) #include +#include +#include #include #include #include @@ -25,9 +27,16 @@ #include #include #include +// std::system_error, thrown when the system refuses a thread. libc++ pulls this in through , +// libstdc++ does not, so leaving it implicit builds on macOS and fails on GCC. +#include #include #include +#if defined(__unix__) || defined(__APPLE__) +#include +#endif + // ABI 4: PkScenario::soc_out may be null, and pk_run_batch was added. The null is the reason this had // to move - Python now passes no SoC buffer for every cached run, and an ABI 3 binary writes to it // unconditionally, so loading one against this Python segfaults on the first prediction rather than @@ -304,12 +313,47 @@ struct PkBatchResult { // each scenario would otherwise heap-allocate ~70KB, and 1200 of those per batch put every worker // thread in contention on the allocator. Measured 4 threads at 0.6x of serial before this, because // the simulation is cheap enough that malloc, not arithmetic, was the bottleneck. +// +// See thread_scratch() below: one of these now serves every batch a thread runs, not one per call. + +// Counts PkScratch constructions for the life of the library, for pk_scratch_construct_count(). The +// suite asserts this stops growing with the number of batches rather than trusting the reuse. +std::atomic g_scratch_constructions{0}; + +// Counts worker threads started for the life of the library, for pk_worker_thread_count(). Thread +// creation measured 7.5us each and a benchmark plan used to start 201,545 of them - 1.5s of a 26s +// plan, which is what the pool exists to stop paying. The suite asserts this stops growing. +std::atomic g_worker_threads_started{0}; + struct PkScratch { std::vector charge_member, export_member; std::vector clipped_start, clipped_end; std::vector clipped_limit; + + PkScratch() + { + g_scratch_constructions.fetch_add(1, std::memory_order_relaxed); + } }; +// One scratch per thread, reused for that thread's lifetime. +// +// Every field is fully rewritten before it is read on each run - build_window_membership assigns +// exactly n_steps entries and the clip helpers clear-then-push - which is the same invariant that +// already let one scratch serve a whole stride within a batch. Extending it across calls means a +// thread grows these buffers once instead of once per call: a benchmark plan issues ~25,000 batch +// calls, and at a median of 6 jobs each the ~70KB allocation was the same order as the simulation it +// wrapped. +// +// Thread-local rather than per-context because nothing here survives a simulation, so it never needs +// to belong to a particular context. A thread that later runs a context with a different n_steps +// simply has the vectors reassigned to the new length. +static PkScratch &thread_scratch() +{ + static thread_local PkScratch scratch; + return scratch; +} + // Deep-copied context storage so Python-side buffers can be freed after create struct ContextStore { std::vector rate_import, rate_export, alert_keep; @@ -1281,8 +1325,7 @@ int32_t pk_run(int64_t handle, const PkScenario *s, PkResult *out) if (!c) { return 1; } - PkScratch scratch; - return pk_run_one(c, s, out, -1, -1, nullptr, nullptr, scratch); // c is the store + return pk_run_one(c, s, out, -1, -1, nullptr, nullptr, thread_scratch()); // c is the store } // Run n_jobs scenarios against one context in a single call. @@ -1317,6 +1360,170 @@ static void run_batch_job(const ContextStore *c, const PkBatchJob &job, PkBatchR out.status = pk_run_one(c, &scenario, &out.result, job.soc_range_start_step, job.soc_range_end_step, &out.soc_range_min, &out.soc_range_max, scratch); } + +// A pool of worker threads that outlives every batch and every context. +// +// pk_run_batch used to build and join a fresh std::vector per call. Measured over one +// benchmark plan that was 201,545 thread creations costing 1.5s - against 3.1s of simulation, so the +// creation cost was cancelling most of what the parallelism won and threading measured 1.6% overall. +// Parking workers on a condition variable replaces each creation with a wake. +// +// Workers park until `generation` changes, then take lane `index` of a stride partition. The calling +// thread takes lane 0 itself, so a six-job batch wakes five workers rather than six and the caller +// does useful work instead of blocking. Nobody holds the mutex while simulating; run_batch_job only +// reads the shared const context and writes its own result slot, which is what made the previous +// per-call threading safe and is unchanged here. +class PkThreadPool { + public: + // Start workers until at least `want` exist. Grows, never shrinks. Returns how many there are. + int32_t reserve_workers(int32_t want) + { + std::lock_guard lock(mutex); + while (static_cast(workers.size()) < want) { + const int32_t lane = static_cast(workers.size()) + 1; // lane 0 is the caller + std::unique_ptr worker(new Worker()); + try { + worker->thread = std::thread(&PkThreadPool::worker_loop, this, lane, worker.get()); + } catch (const std::system_error &) { + // Out of thread resources - keep what we have and let the caller use fewer lanes. + break; + } + workers.push_back(std::move(worker)); + g_worker_threads_started.fetch_add(1, std::memory_order_relaxed); + } + return static_cast(workers.size()); + } + + // Run jobs[0..n_jobs) across `use` lanes, the calling thread taking lane 0. `use` must be at + // least 1 and no more than worker_count() + 1. + void run(const ContextStore *c, const PkBatchJob *jobs, int32_t n_jobs, PkBatchResult *results, int32_t use) + { + { + std::lock_guard lock(mutex); + cur_ctx = c; + cur_jobs = jobs; + cur_results = results; + cur_n_jobs = n_jobs; + cur_use = use; + outstanding = use - 1; + // Each worker has its own flag and its own condition variable, so exactly the lanes this + // batch needs are woken. Broadcasting instead would wake every worker in the pool - with + // a median batch of six jobs against sixteen workers that was ~250,000 wakeups per plan + // spent entirely on threads that immediately went back to sleep. + for (int32_t lane = 1; lane < use; lane++) { + workers[lane - 1]->has_work = true; + } + } + for (int32_t lane = 1; lane < use; lane++) { + workers[lane - 1]->ready.notify_one(); + } + + PkScratch &scratch = thread_scratch(); + for (int32_t i = 0; i < n_jobs; i += use) { + run_batch_job(c, jobs[i], results[i], scratch); + } + + std::unique_lock lock(mutex); + work_done.wait(lock, [this] { return outstanding == 0; }); + } + + private: + // One parked worker. The flag is written under the pool mutex before its condition variable is + // signalled, so a wake cannot be lost even if the worker has not re-parked yet. + struct Worker { + std::condition_variable ready; + bool has_work = false; + std::thread thread; + }; + + void worker_loop(int32_t lane, Worker *self) + { + std::unique_lock lock(mutex); + for (;;) { + self->ready.wait(lock, [self] { return self->has_work; }); + self->has_work = false; + const ContextStore *c = cur_ctx; + const PkBatchJob *jobs = cur_jobs; + PkBatchResult *results = cur_results; + const int32_t n_jobs = cur_n_jobs; + const int32_t use = cur_use; + lock.unlock(); + + PkScratch &scratch = thread_scratch(); + for (int32_t i = lane; i < n_jobs; i += use) { + run_batch_job(c, jobs[i], results[i], scratch); + } + + lock.lock(); + if (--outstanding == 0) { + work_done.notify_one(); + } + } + } + + std::mutex mutex; + std::condition_variable work_done; // the caller waits here + const ContextStore *cur_ctx = nullptr; + const PkBatchJob *cur_jobs = nullptr; + PkBatchResult *cur_results = nullptr; + int32_t cur_n_jobs = 0; + int32_t cur_use = 1; + int32_t outstanding = 0; + std::vector> workers; +}; + +// The pool is created on first threaded use and deliberately never destroyed: a static destructor +// running while CPython tears the process down risks a deadlock for no benefit, and the OS reclaims +// the threads at exit. +std::atomic g_pool{nullptr}; +std::mutex g_pool_create_mutex; +// Held for a whole batch. Python can enter pk_run_batch from two threads at once because ctypes +// releases the GIL, and one shared pool cannot serve two batches concurrently - its published job +// state would race. Nothing in predbat does this today; serialising is the safe answer if it ever does. +std::mutex g_dispatch_mutex; + +#if defined(__unix__) || defined(__APPLE__) +// Threads do not survive fork(), so a child would inherit a pool whose workers do not exist and hang +// waiting for them. The prepare/parent pair keep the dispatch mutex consistent across the fork, and +// the child drops the pool so its next batch builds a fresh one. Nothing here allocates or takes a +// new lock, which is what the child handler is allowed to do; the dead pool leaks in the child. +void pk_atfork_prepare() +{ + g_dispatch_mutex.lock(); +} + +void pk_atfork_parent() +{ + g_dispatch_mutex.unlock(); +} + +void pk_atfork_child() +{ + g_dispatch_mutex.unlock(); + g_pool.store(nullptr, std::memory_order_relaxed); +} +#endif + +// Fetch the pool, creating it on first use. +static PkThreadPool *pool_singleton() +{ + PkThreadPool *pool = g_pool.load(std::memory_order_acquire); + if (pool) { + return pool; + } + std::lock_guard lock(g_pool_create_mutex); + pool = g_pool.load(std::memory_order_relaxed); + if (!pool) { +#if defined(__unix__) || defined(__APPLE__) + static std::once_flag atfork_once; + std::call_once(atfork_once, [] { pthread_atfork(pk_atfork_prepare, pk_atfork_parent, pk_atfork_child); }); +#endif + pool = new PkThreadPool(); + g_pool.store(pool, std::memory_order_release); + } + return pool; +} + // Run n_jobs scenarios against one context in a single call. // // The context lookup and the Python/C boundary are paid once for a whole fan-out rather than once @@ -1338,30 +1545,50 @@ int32_t pk_run_batch(int64_t handle, const PkBatchJob *jobs, int32_t n_jobs, PkB // Scenarios are independent - each reads the shared const context and writes only its own result // slot - so they are split across threads by stride. if (n_threads > 1 && n_jobs > 1) { - const int32_t use = n_threads < n_jobs ? n_threads : n_jobs; - std::vector pool; - pool.reserve(use); - for (int32_t t = 0; t < use; t++) { - pool.emplace_back([c, jobs, results, n_jobs, use, t]() { - PkScratch scratch; - for (int32_t i = t; i < n_jobs; i += use) { - run_batch_job(c, jobs[i], results[i], scratch); - } - }); + std::lock_guard dispatch(g_dispatch_mutex); + int32_t use = n_threads < n_jobs ? n_threads : n_jobs; + PkThreadPool *pool = pool_singleton(); + // Clamp to the lanes that actually exist: reserve_workers stops early if the system refuses a + // thread, and a batch must never wait on a worker that was never started. + const int32_t lanes = pool->reserve_workers(use - 1) + 1; + if (use > lanes) { + use = lanes; } - for (auto &th : pool) { - th.join(); + if (use > 1) { + pool->run(c, jobs, n_jobs, results, use); + return 0; } - return 0; + // No workers available - fall through and run the batch inline. } - PkScratch scratch; for (int32_t i = 0; i < n_jobs; i++) { - run_batch_job(c, jobs[i], results[i], scratch); + run_batch_job(c, jobs[i], results[i], thread_scratch()); } return 0; } +// Test hook: how many PkScratch instances have been constructed since the library was loaded. +// +// The scratch buffers are the only heap allocation a simulation makes, and the kernel's whole +// batching win rests on not paying them per call - so the suite asserts this counter stops growing +// with the number of batches rather than trusting the code to be reusing them. +int64_t pk_scratch_construct_count(void) +{ + return g_scratch_constructions.load(std::memory_order_relaxed); +} + + +// Test hook: how many worker threads have been started since the library was loaded. +// +// Batches used to build and join a fresh set of threads on every call. The suite asserts this count +// stops growing once the pool is warm, because a thread creation costs several microseconds against +// a median batch of six jobs - it was cancelling the parallelism it was there to provide. +int64_t pk_worker_thread_count(void) +{ + return g_worker_threads_started.load(std::memory_order_relaxed); +} + + // Test hook: sweep SoC densely and confirm the precomputed bucket boundaries give exactly the same // percent as the round_py path they replace. Returns the number of disagreements (0 = equivalent). int32_t pk_verify_soc_percent_table(double soc_max, int32_t samples) diff --git a/apps/predbat/prediction_kernel_lib_aarch64.so b/apps/predbat/prediction_kernel_lib_aarch64.so index d226c555f..eda553983 100755 Binary files a/apps/predbat/prediction_kernel_lib_aarch64.so and b/apps/predbat/prediction_kernel_lib_aarch64.so differ diff --git a/apps/predbat/prediction_kernel_lib_armv7l.so b/apps/predbat/prediction_kernel_lib_armv7l.so index 97a7bb0dc..b26eda510 100755 Binary files a/apps/predbat/prediction_kernel_lib_armv7l.so and b/apps/predbat/prediction_kernel_lib_armv7l.so differ diff --git a/apps/predbat/prediction_kernel_lib_darwin_arm64.so b/apps/predbat/prediction_kernel_lib_darwin_arm64.so index 43e754877..0431deb3d 100755 Binary files a/apps/predbat/prediction_kernel_lib_darwin_arm64.so and b/apps/predbat/prediction_kernel_lib_darwin_arm64.so differ diff --git a/apps/predbat/prediction_kernel_lib_darwin_x86_64.so b/apps/predbat/prediction_kernel_lib_darwin_x86_64.so index 7563cbc48..7300ca9bd 100755 Binary files a/apps/predbat/prediction_kernel_lib_darwin_x86_64.so and b/apps/predbat/prediction_kernel_lib_darwin_x86_64.so differ diff --git a/apps/predbat/prediction_kernel_lib_i686.so b/apps/predbat/prediction_kernel_lib_i686.so index 9ced39b3a..05939aabc 100755 Binary files a/apps/predbat/prediction_kernel_lib_i686.so and b/apps/predbat/prediction_kernel_lib_i686.so differ diff --git a/apps/predbat/prediction_kernel_lib_x86_64.so b/apps/predbat/prediction_kernel_lib_x86_64.so index 9ef83b0c5..9cb9287e0 100755 Binary files a/apps/predbat/prediction_kernel_lib_x86_64.so and b/apps/predbat/prediction_kernel_lib_x86_64.so differ diff --git a/apps/predbat/tests/test_kernel_parity.py b/apps/predbat/tests/test_kernel_parity.py index 3183e6cdf..bdcd49025 100644 --- a/apps/predbat/tests/test_kernel_parity.py +++ b/apps/predbat/tests/test_kernel_parity.py @@ -26,7 +26,11 @@ import gc import os import random +import signal import subprocess +import sys +import threading +import time import prediction_kernel from const import PV_SCENARIO_NOMINAL, PV_SCENARIO_PV10, PV_SCENARIO_PV90 @@ -1124,6 +1128,319 @@ def run_soc_percent_table_tests(): return failed +def run_scratch_reuse_tests(my_predbat, batches=10): + """Check a thread builds its scratch buffers once, not once per batch, returns True on failure. + + The scratch vectors are the only heap allocation a simulation makes - roughly 70KB once they have + grown to n_steps - and a benchmark plan issues around 25,000 batch calls, so paying that per call + is the difference between the allocator and the arithmetic being the bottleneck. The kernel + exposes pk_scratch_construct_count for this: the count must stop growing with the number of + batches rather than tracking it. + """ + print("**** Running kernel scratch reuse tests ****") + lib = prediction_kernel.KERNEL_LIB + if not lib or not hasattr(lib, "pk_scratch_construct_count"): + print("SKIP: kernel does not expose pk_scratch_construct_count") + return False + + lib.pk_scratch_construct_count.restype = ctypes.c_int64 + lib.pk_scratch_construct_count.argtypes = [] + + prediction, jobs = build_batch_jobs(my_predbat, random.Random(99), 4) + if not prediction.kernel_handle: + print("ERROR: scratch reuse kernel context creation failed") + return True + + # One batch first, so any first-touch allocation is already paid before the count is taken + prediction_kernel.run_prediction_kernel_batch(prediction, jobs, 1) + before = lib.pk_scratch_construct_count() + for _ in range(batches): + if prediction_kernel.run_prediction_kernel_batch(prediction, jobs, 1) is None: + print("ERROR: scratch reuse batch call failed") + return True + grew = lib.pk_scratch_construct_count() - before + + if grew: + print("ERROR: {} batches built {} scratch buffers - they should be reused, not rebuilt per call".format(batches, grew)) + return True + print("Scratch reuse: {} batches on one thread built 0 further scratch buffers".format(batches)) + return False + + +def run_scratch_cross_context_tests(my_predbat, rounds=3): + """Check a reused scratch cannot leak between contexts of different sizes, returns True on failure. + + One scratch now serves every batch a thread ever runs, and Predbat builds a fresh context each + plan cycle - with a different n_steps whenever forecast_minutes changes. The buffers are only safe + to reuse because every field is fully rewritten before it is read: build_window_membership assigns + exactly n_steps entries and the clip helpers clear before pushing. If that ever stopped holding, a + large context followed by a small one would read the large one's tail. + + So the contexts are deliberately run largest-first and then interleaved, and each must keep + returning what it returned on its own. + """ + print("**** Running kernel scratch cross-context tests ****") + if not prediction_kernel.KERNEL_HAS_BATCH: + print("SKIP: kernel does not expose pk_run_batch") + return False + + saved_forecast = my_predbat.forecast_minutes + try: + cases = [] + # Largest first: a later, smaller context is the one that would read a stale tail + for forecast_minutes in (48 * 60, 12 * 60): + my_predbat.forecast_minutes = forecast_minutes + prediction, jobs = build_batch_jobs(my_predbat, random.Random(forecast_minutes), 6) + if not prediction.kernel_handle: + print("ERROR: cross-context kernel context creation failed for {} minutes".format(forecast_minutes)) + return True + cases.append((forecast_minutes, prediction, jobs, build_batch_reference(prediction, jobs))) + + failed = False + for round_n in range(rounds): + for forecast_minutes, prediction, jobs, reference in cases: + batched = prediction_kernel.run_prediction_kernel_batch(prediction, jobs, 1) + if batched is None: + print("ERROR: cross-context batch call failed for {} minutes".format(forecast_minutes)) + return True + for index, (result_tuple, soc_range_min, soc_range_max) in enumerate(batched): + single, (expect_min, expect_max) = reference[index] + if result_tuple is None: + print("ERROR: round {} {} minutes job {} reported a non-zero status".format(round_n, forecast_minutes, index)) + failed = True + continue + for field, name in enumerate(RESULT_NAMES): + if result_tuple[field] != single[field]: + print("ERROR: round {} {} minutes job {} differs on {}: got {} want {}".format(round_n, forecast_minutes, index, name, result_tuple[field], single[field])) + failed = True + if soc_range_min != expect_min or soc_range_max != expect_max: + print("ERROR: round {} {} minutes job {} SoC range {} {} want {} {}".format(round_n, forecast_minutes, index, soc_range_min, soc_range_max, expect_min, expect_max)) + failed = True + if failed: + break + + if not failed: + print("Scratch cross-context: {} rounds interleaving {} and {} minute contexts, all identical".format(rounds, cases[0][0], cases[1][0])) + return failed + finally: + my_predbat.forecast_minutes = saved_forecast + + +def run_worker_reuse_tests(my_predbat, batches=10, n_threads=4): + """Check threaded batches reuse parked workers instead of starting new ones, returns True on failure. + + A benchmark plan issues ~25,000 threaded batch calls with a median of six jobs each. Starting a + thread per lane per call measured 201,545 threads and 1.5s of a 26s plan - more than the + parallelism was returning, which is why threading was worth 1.6% before the pool. The kernel + exposes pk_worker_thread_count for this: once warm, the count must stop growing with the number of + batches. + """ + print("**** Running kernel worker reuse tests ****") + lib = prediction_kernel.KERNEL_LIB + if not lib or not hasattr(lib, "pk_worker_thread_count"): + print("SKIP: kernel does not expose pk_worker_thread_count") + return False + + lib.pk_worker_thread_count.restype = ctypes.c_int64 + lib.pk_worker_thread_count.argtypes = [] + + prediction, jobs = build_batch_jobs(my_predbat, random.Random(1234), 8) + if not prediction.kernel_handle: + print("ERROR: worker reuse kernel context creation failed") + return True + + # Warm first, so the pool has grown to n_threads before the count is taken + prediction_kernel.run_prediction_kernel_batch(prediction, jobs, n_threads) + before = lib.pk_worker_thread_count() + for _ in range(batches): + if prediction_kernel.run_prediction_kernel_batch(prediction, jobs, n_threads) is None: + print("ERROR: worker reuse batch call failed") + return True + grew = lib.pk_worker_thread_count() - before + + if grew: + print("ERROR: {} batches at {} threads started {} more workers - they should be reused, not restarted per call".format(batches, n_threads, grew)) + return True + print("Worker reuse: {} batches at {} threads started 0 further workers".format(batches, n_threads)) + return False + + +def compare_batch_to_reference(label, batched, reference): + """Compare a batch's results against a per-job pk_run reference, returns True on failure""" + failed = False + for index, (result_tuple, soc_range_min, soc_range_max) in enumerate(batched): + single, (expect_min, expect_max) = reference[index] + if result_tuple is None: + print("ERROR: {} job {} reported a non-zero status".format(label, index)) + failed = True + continue + for field, name in enumerate(RESULT_NAMES): + if result_tuple[field] != single[field]: + print("ERROR: {} job {} differs on {}: got {} want {}".format(label, index, name, result_tuple[field], single[field])) + failed = True + if soc_range_min != expect_min or soc_range_max != expect_max: + print("ERROR: {} job {} SoC range {} {} want {} {}".format(label, index, soc_range_min, soc_range_max, expect_min, expect_max)) + failed = True + return failed + + +def run_pool_lane_tests(my_predbat): + """Check lane allocation across batch and pool sizes, returns True on failure. + + The pool is sized by the largest thread count it has been asked for and never shrinks, so most + batches use fewer lanes than there are workers - the median batch is six jobs against a pool built + for more. Idle workers must sit out without touching the completion count, and a batch must never + wait on a lane that was never dispatched. These combinations walk a batch smaller than the pool, + the pool growing, the pool being asked for less than it has, and the single-job and single-thread + bypasses. + """ + print("**** Running kernel pool lane tests ****") + if not prediction_kernel.KERNEL_HAS_BATCH: + print("SKIP: kernel does not expose pk_run_batch") + return False + + prediction, jobs = build_batch_jobs(my_predbat, random.Random(2468), 8) + if not prediction.kernel_handle: + print("ERROR: pool lane kernel context creation failed") + return True + reference = build_batch_reference(prediction, jobs) + + failed = False + for n_threads, n_jobs in ((16, 3), (8, 8), (2, 8), (16, 8), (4, 1), (1, 8), (3, 8)): + subset = jobs[:n_jobs] + batched = prediction_kernel.run_prediction_kernel_batch(prediction, subset, n_threads) + if batched is None: + print("ERROR: pool lane batch failed at {} threads {} jobs".format(n_threads, n_jobs)) + return True + failed |= compare_batch_to_reference("threads={} jobs={}".format(n_threads, n_jobs), batched, reference[:n_jobs]) + + if not failed: + print("Pool lanes: 7 thread/job combinations all bit-identical to pk_run") + return failed + + +def run_pool_concurrency_tests(my_predbat, rounds=400, n_threads=8): + """Check two Python threads can dispatch batches at once, returns True on failure. + + ctypes releases the GIL, so predbat could enter pk_run_batch from two threads simultaneously. One + shared pool cannot serve two batches at once - the published job pointers would race - so dispatch + is serialised by a mutex. Nothing does this today, which is why it needs a test rather than an + assumption: each thread checks its own job set against its own reference, so a crossed batch shows + up as wrong values, not just a crash. + + The round count is load-bearing and was chosen by mutation, not taste. With the dispatch mutex + deleted, 25 rounds over 4 lanes passes happily - the GIL keeps the two callers from overlapping + often enough for the race to land - while 400 rounds over 8 lanes deadlocks within seconds and is + caught by the join timeout below. The healthy case costs no measurable time either way, so do not + trim these numbers to make the suite look faster. + """ + print("**** Running kernel pool concurrency tests ****") + if not prediction_kernel.KERNEL_HAS_BATCH: + print("SKIP: kernel does not expose pk_run_batch") + return False + + sets = [] + for seed in (11, 22): + prediction, jobs = build_batch_jobs(my_predbat, random.Random(seed), 6) + if not prediction.kernel_handle: + print("ERROR: pool concurrency kernel context creation failed") + return True + sets.append((prediction, jobs, build_batch_reference(prediction, jobs))) + + errors = [] + + def hammer(label, prediction, jobs, reference): + """Run many batches on one context and record any mismatch against its own reference""" + for _ in range(rounds): + batched = prediction_kernel.run_prediction_kernel_batch(prediction, jobs, n_threads) + if batched is None: + errors.append("{}: batch call failed".format(label)) + return + if compare_batch_to_reference(label, batched, reference): + errors.append("{}: result mismatch".format(label)) + return + + workers = [threading.Thread(target=hammer, args=("concurrent-{}".format(n), prediction, jobs, reference)) for n, (prediction, jobs, reference) in enumerate(sets)] + for worker in workers: + worker.start() + for worker in workers: + worker.join(timeout=120) + for worker in workers: + if worker.is_alive(): + print("ERROR: concurrent dispatch did not finish within 120s - suspect a deadlock") + return True + + if errors: + for error in errors: + print("ERROR: {}".format(error)) + return True + print("Pool concurrency: 2 threads x {} batches, all bit-identical to pk_run".format(rounds)) + return False + + +def run_pool_fork_tests(my_predbat, n_threads=4, timeout_s=60): + """Check a batch still runs in a forked child, returns True on failure. + + Threads do not survive fork(), so a child inherits a pool whose workers do not exist. Without the + pthread_atfork handler that drops the pool, the child's first batch waits forever on lanes that + will never report - a hang, not a crash, which is why this test kills the child on a deadline and + treats the timeout as the failure rather than waiting on it. + """ + print("**** Running kernel pool fork tests ****") + if not prediction_kernel.KERNEL_HAS_BATCH: + print("SKIP: kernel does not expose pk_run_batch") + return False + + if not hasattr(os, "fork"): + print("SKIP: platform has no fork()") + return False + + prediction, jobs = build_batch_jobs(my_predbat, random.Random(3690), 6) + if not prediction.kernel_handle: + print("ERROR: pool fork kernel context creation failed") + return True + reference = build_batch_reference(prediction, jobs) + + # Warm the pool in the parent, so the child inherits one whose workers are gone + if prediction_kernel.run_prediction_kernel_batch(prediction, jobs, n_threads) is None: + print("ERROR: pool fork parent warm-up batch failed") + return True + + sys.stdout.flush() + sys.stderr.flush() + pid = os.fork() + if pid == 0: + # Child: os._exit throughout, so the parent's buffers are never flushed twice + try: + batched = prediction_kernel.run_prediction_kernel_batch(prediction, jobs, n_threads) + if batched is None: + os._exit(3) + os._exit(1 if compare_batch_to_reference("fork-child", batched, reference) else 0) + except BaseException: + os._exit(2) + + deadline = time.time() + timeout_s + status = None + while time.time() < deadline: + waited, raw_status = os.waitpid(pid, os.WNOHANG) + if waited == pid: + status = raw_status + break + time.sleep(0.05) + + if status is None: + os.kill(pid, signal.SIGKILL) + os.waitpid(pid, 0) + print("ERROR: forked child did not finish within {}s - the inherited pool deadlocked".format(timeout_s)) + return True + if not os.WIFEXITED(status) or os.WEXITSTATUS(status) != 0: + print("ERROR: forked child exited with status {} (1 = mismatch, 2 = exception, 3 = batch failed)".format(status)) + return True + + print("Pool fork: a batch in a forked child matched pk_run") + return False + + def run_kernel_parity_tests(my_predbat): """Compare the C++ prediction kernel against the Python engine, returns True on failure""" print("**** Running kernel parity tests ****") @@ -1145,6 +1462,18 @@ def run_kernel_parity_tests(my_predbat): failed |= run_batch_parity_tests(my_predbat) if not failed: failed |= run_batch_window_variant_tests(my_predbat) + if not failed: + failed |= run_scratch_reuse_tests(my_predbat) + if not failed: + failed |= run_scratch_cross_context_tests(my_predbat) + if not failed: + failed |= run_worker_reuse_tests(my_predbat) + if not failed: + failed |= run_pool_lane_tests(my_predbat) + if not failed: + failed |= run_pool_concurrency_tests(my_predbat) + if not failed: + failed |= run_pool_fork_tests(my_predbat) finally: restore_scenario_state(my_predbat, state) diff --git a/apps/predbat/tests/test_prediction_batch.py b/apps/predbat/tests/test_prediction_batch.py index 36bcb3184..3bfd2046b 100644 --- a/apps/predbat/tests/test_prediction_batch.py +++ b/apps/predbat/tests/test_prediction_batch.py @@ -24,6 +24,7 @@ import prediction_batch from const import PREDICT_STEP, PV_SCENARIO_NOMINAL, PV_SCENARIO_PV10 +from plan import resolve_batch_threads from prediction import Prediction from prediction_kernel import create_kernel_context from tests.test_kernel_parity import apply_random_scenario, kernel_available, make_step_data, make_windows, restore_scenario_state, snapshot_scenario_state @@ -511,9 +512,42 @@ def test_batch_results_match_their_own_job(my_predbat): return failed +def test_batch_thread_count_resolution(): + """Check how the threads setting maps onto kernel lanes, returns True on failure. + + 'auto' takes the core count uncapped. A cap was measured and rejected: on a fast machine the + curve is flat and peaks slightly below the core count, but with each job made eight times dearer + - how a machine where the kernel dominates behaves - it stops turning over, and a cap of 4 then + costs 10.7% against 1.3% for no cap at all. The zero and one cases both mean serial, which is what + annual.py and the benchmark harness rely on. + """ + print("**** Running batch thread count resolution tests ****") + failed = False + cases = [ + ("auto", 2, 2, "auto follows the core count"), + ("auto", 16, 16, "auto is not capped"), + ("auto", 64, 64, "auto is not capped on a large machine either"), + ("auto", 0, 1, "a nonsense core count still leaves one usable lane"), + (0, 32, 1, "0 means serial"), + (1, 32, 1, "1 means serial"), + (2, 32, 2, "an explicit value is honoured"), + (12, 32, 12, "an explicit value above the core count is honoured"), + ] + for threads, cores, expected, why in cases: + got = resolve_batch_threads(threads, cores) + if got != expected: + print("ERROR: threads={} cores={} gave {} lanes, expected {} ({})".format(threads, cores, got, expected, why)) + failed = True + + if not failed: + print("Batch thread count resolution tests passed") + return failed + + def run_prediction_batch_tests(my_predbat): """Run every batched prediction test, returns True on failure""" failed = test_export_trial_does_not_mutate_caller_window(my_predbat) + failed |= test_batch_thread_count_resolution() failed |= test_batch_state_exists_without_a_base(my_predbat) available, required_failure = kernel_available() diff --git a/docs/superpowers/specs/2026-08-16-kernel-thread-pool-design.md b/docs/superpowers/specs/2026-08-16-kernel-thread-pool-design.md new file mode 100644 index 000000000..7c4d3986b --- /dev/null +++ b/docs/superpowers/specs/2026-08-16-kernel-thread-pool-design.md @@ -0,0 +1,267 @@ +# Persistent scratch and thread pool in the prediction kernel + +**Status:** implemented. Three things below were changed by measurement during implementation and are +recorded in place: stage 1 is a no-op on its own, broadcasting to wake workers was the wrong choice, +and the `threads: auto` cap this design would have added was measured and rejected. See "What +measurement changed" at the end. + +**Branch:** `perf/kernel-thread-pool`, stacked on `perf/kernel-batch-abi` (PR #4540). Neither +`pk_run_batch` nor `PkScratch` exists on `main` — both arrive with that PR — so this work cannot be +based on `main`. The follow-up PR takes `perf/kernel-batch-abi` as its base so its diff shows only +this change, and is retargeted to `main` once #4540 merges. + +## Why + +PR #4540 replaced the per-scenario prediction fan-out with a batch that reaches the C++ kernel in one +`pk_run_batch` call. It made planning 35-37% faster, but enabling kernel threading on top of it +bought only 1.6% — inside the run-to-run noise band. + +A census over one 20-scenario benchmark run explains why: + +```text +25,091 pk_run_batch calls carrying 253,899 jobs +mean 10.12 jobs/call, median 6, p95 24, max 363 +97.4% of calls carry fewer than 32 jobs +``` + +The median batch is six jobs of roughly 30us each — about 180us of simulation. Against that, +`pk_run_batch` builds and joins a fresh `std::vector` on every call, and the serial path +constructs a fresh `PkScratch` whose first use heap-allocates roughly 70KB. Both costs are paid per +call, and at these batch sizes both are the same order as the work they wrap. + +The kernel's own comment already records that this class of cost dominates: scratch reuse was +introduced because "each scenario would otherwise heap-allocate ~70KB, and 1200 of those per batch +put every worker thread in contention on the allocator. Measured 4 threads at 0.6x of serial before +this, because the simulation is cheap enough that malloc, not arithmetic, was the bottleneck." + +This design removes both per-call costs. It is deliberately staged, because the two halves carry very +different risk and the first may be enough on its own. + +## Scope + +**In scope:** `apps/predbat/prediction_kernel.cpp`, its tests, and the six shipped binaries. + +**Out of scope:** any Python change. `n_threads` stays a per-call argument, the struct layouts do not +move, and the function signatures are unchanged, so `prediction_kernel.py` is untouched. + +**Not an ABI or parity change.** Results are bit-identical before and after, so neither +`PK_ABI_VERSION` nor `PK_PARITY_REVISION` is bumped. A binary built before this change still loads +and runs correctly, just without the speedup. The six architecture binaries are still rebuilt and +shipped. + +## Stage 1 — one scratch per thread + +`PkScratch` is constructed at three sites: `pk_run` (prediction_kernel.cpp:1279), each worker lambda +in `pk_run_batch` (:1341), and the serial batch loop (:1352). All three become one accessor: + +```cpp +// One scratch per thread, reused for that thread's lifetime. Every field is fully rewritten before +// it is read on each run - build_window_membership assigns exactly n_steps entries and the clip +// helpers clear-then-push - which is the same invariant that already lets one scratch serve a whole +// stride. Extending it across calls means a thread allocates these buffers once, not once per call. +// Thread-local rather than per-context because nothing here survives a simulation, so it never needs +// to belong to a particular context. +static PkScratch &thread_scratch() +{ + static thread_local PkScratch scratch; + return scratch; +} +``` + +Each of the three sites drops its local and passes `thread_scratch()` instead. + +### The risk this introduces + +Reuse within a call spans one context, so `n_steps` is constant. Reuse across calls spans contexts, +and Predbat builds a new context every plan cycle — with a different `n_steps` whenever +`forecast_minutes` changes. `build_window_membership` assigns exactly `n_steps` entries and the clip +vectors clear before pushing, so a shrink cannot expose stale tail data through any read path. + +That is an argument, not a proof, so it is pinned by a test (see Testing). + +### Expected effect + +Removes roughly 25,000 allocations of ~70KB per plan from the serial path — which is the default for +`threads: 0`, for `annual.py`, and for the whole test harness. Independently measurable and +independently shippable. + +## Stage 2 — one pool for the process + +A single pool, allocated on first threaded use and never destroyed, holding workers parked on a +condition variable. + +```cpp +class PkThreadPool { +public: + void reserve_workers(int32_t want); // grows, never shrinks + void run(const ContextStore *c, const PkBatchJob *jobs, int32_t n_jobs, PkBatchResult *results, int32_t use); +private: + std::mutex mutex; + std::condition_variable work_ready; // workers park here + std::condition_variable work_done; // the caller waits here + const ContextStore *cur_ctx = nullptr; + const PkBatchJob *cur_jobs = nullptr; + PkBatchResult *cur_results = nullptr; + int32_t cur_n_jobs = 0; + int32_t cur_use = 1; + uint64_t generation = 0; // bumped per batch; workers wake when it changes + int32_t outstanding = 0; // lanes still running + std::vector workers; +}; +``` + +### Dispatch + +The lane count is `use = min(n_threads, n_jobs, workers.size() + 1)` — bounded by what the caller +asked for, by how much work there is, and by how many workers actually exist (see Thread creation +failure). The `+ 1` is the calling thread, which takes a lane itself. + +The caller, holding `mutex`: + +1. publishes the batch — context, jobs, results, `n_jobs` and `use` +2. sets `outstanding = use - 1` +3. bumps `generation` +4. broadcasts on `work_ready` + +then releases the mutex and **runs lane 0 itself**, striding `i = 0; i < n_jobs; i += use`. This +matters at these batch sizes: six jobs wakes five workers rather than six, and the caller does useful +work instead of blocking. Finally it waits on `work_done` until `outstanding` reaches zero. + +Each worker parks on `work_ready` until `generation` changes, records the new generation +unconditionally so it cannot spin, and participates only if its lane index is `< cur_use`. Workers +outside that range park again without touching `outstanding`. A participating worker copies the batch +pointers locally, releases the mutex, strides `i = lane; i < n_jobs; i += use`, then re-acquires the +mutex and notifies `work_done` if it was the last one out. + +Workers simulate through `thread_scratch()` from stage 1, so a persistent worker allocates its +buffers on its first batch and reuses them for the life of the process. Stage 1 replaced the scratch +local in the per-call worker lambda; stage 2 replaces that lambda with a parked worker, and the +accessor carries over unchanged. + +Nobody holds the mutex while simulating. The partition is the same stride the current code uses, and +`run_batch_job` already only reads the const context and writes its own result slot — the property +that makes today's threading safe is unchanged. + +### Bypass + +`n_threads <= 1 || n_jobs <= 1` runs inline exactly as today and never touches the pool, so +`threads: 0`, `annual.py` and the test harness never allocate a worker. + +### Sizing + +`reserve_workers` grows to whatever `n_threads` asks for and never shrinks. A later, smaller request +simply wakes fewer lanes. + +## Error handling + +**Fork.** Threads do not survive `fork()`, so a child would inherit a pool whose workers do not exist +and hang waiting on them. Nothing in Predbat forks today — PR #4540 deleted the last thing that did — +but `hass.py` still calls `set_start_method("fork")`, so a future feature that spawns a process would +trip it. A `pthread_atfork` child handler sets the pool pointer to null: no allocation, no locking, +async-signal-safe. The child lazily builds a fresh pool on its next batch, and the dead object leaks +in the child's address space, which is standard and costs a few hundred bytes. Registered once via +`std::call_once` before the pool is first created, guarded by `#if defined(__unix__) || defined(__APPLE__)`. + +**Concurrent callers.** Two Python threads can enter `pk_run_batch` at once, because ctypes releases +the GIL. With one shared pool their published state would race, so a dispatch mutex is held for the +whole batch and concurrent callers serialise. Today they would each spawn private threads and run in +parallel, so this is a regression in a case nothing exercises; a data race is the worse option. + +**Thread creation failure.** `std::thread` throws `std::system_error` under resource exhaustion. +`reserve_workers` catches it, keeps whatever workers were created, and `run` clamps `use` to +`workers.size() + 1` — falling all the way back to inline if none exist. A home-automation daemon +should degrade, not crash. + +**Shutdown.** The pool is never destroyed, deliberately. A static destructor running while CPython +tears down risks a deadlock for no benefit; the OS reclaims the threads at exit. + +**Per-job failures** are unchanged: each job's `status` field still carries its own result, so one +malformed scenario cannot discard the fan-out. + +## Testing + +Every new test is mutation-checked — break the thing deliberately, confirm the test fails, restore. +That method found three real defects on PR #4540, two of them tests that could not fail. + +| Pins | Test | +|---|---| +| Stage 1's risk | Alternate two contexts with different `forecast_minutes` on one thread; each must match its single-context result. Stale tail data or a mis-sized buffer appears here and nowhere else. | +| Worker reuse | Run the 1/2/4/8-thread parity sweep twice. The first batch creates workers, the second wakes parked ones — different paths, and "works once, breaks on reuse" is the classic pool bug. | +| Lane arithmetic | `n_jobs < n_threads`: a 3-job batch at 16 threads, so `use = 3` and thirteen workers must park without touching `outstanding`. This is the median-6 production shape. | +| Grow then wake fewer | Call at 8 threads, then 2, then 16. | +| The dispatch mutex | Two Python threads calling `pk_run_batch` at once; results must match serial. | +| The `pthread_atfork` handler | `os.fork()`, run a batch in the child, compare against the parent's result, check the child's exit status. Skipped where fork is unavailable. Without this the handler is unverified code whose whole job is preventing a hang. | + +Existing gates that must continue to hold: the `kernel_parity` sweeps, the 20-scenario byte-identical +run (`+0.0000` on metric, cost and all three PV futures), and the full suite. + +## Measurement + +Interleaved, best of 3 per side, on one machine in one sitting — the method used for PR #4540's +numbers. + +- **Stage 1** on the serial benchmark (`threads: 0`), which is where the 25,000 allocations are. +- **Stage 2** on the threaded benchmark, with serial re-checked to confirm it has not regressed. + +Each stage is measured against the tree it modifies, so the two effects are attributable separately. +If stage 1 recovers most of the available time, stage 2 can be dropped rather than accepting its +concurrency risk for a small remainder. + +**Honest ceiling.** This turns a thread creation (tens of microseconds) into a wake (a few +microseconds), so threading can pay where it currently cannot. But the upside is still bounded by the +kernel's share of plan time, so single-digit to low-teens percent is the realistic expectation, not +anything resembling a 16x. + +## Deliverables + +- `apps/predbat/prediction_kernel.cpp`: `thread_scratch()`, `PkThreadPool`, the atfork handler, and + `pk_run_batch` rewired onto them. +- `apps/predbat/tests/test_kernel_parity.py`: the six tests above. +- All six shipped binaries rebuilt with `build_kernel_cross.sh` and confirmed to load. +- No change to any Python module. + +## What measurement changed + +Three parts of the design above did not survive contact with a benchmark. They are left in place so +the reasoning can be compared against what actually happened. + +**Stage 1 is a no-op on its own.** Removing ~25,000 allocations of ~70KB per plan changed nothing: +26.291s before, 26.344s after, interleaved best of 3 against ~1.8% run-to-run spread. The premise was +a misreading of the kernel's own comment, which recorded the allocator being the bottleneck under +four threads contending — a different problem from one thread repeatedly allocating and freeing the +same-sized block, which never leaves the allocator's thread cache. Stage 1 is kept only because the +pool's parked workers need per-thread scratch, and because its two tests are worth having. + +**Broadcasting to wake workers cost most of the win.** The design says "broadcasts on `work_ready`". +Built that way the pool was worth 1.3% (25.833s → 25.489s), because a six-job batch woke all fifteen +workers and eleven of them went straight back to sleep — roughly 250,000 wasted wakeups per plan. +Giving each worker its own flag and condition variable, so exactly the needed lanes wake, took it to +3.7% (26.008s → 25.056s). The shipped code does that; the single `work_ready` variable in the sketch +above does not exist. + +**The `threads: auto` cap was measured and rejected.** With the pool in place the fast-machine curve +peaks below the core count — serial 26.33s, 4 threads 24.89s, 6 threads 24.71s, 8 threads 24.91s, +16 threads 25.04s — which argues for capping `auto`. Re-running with each job made eight times dearer, +which is how a machine where the kernel dominates behaves, the curve stops turning over: 48.92s +serial, 32.03s at 4, 29.98s at 6, 29.85s at 8, 28.94s at 16. Capping at 4 therefore costs 0.7% on a +fast machine but 10.7% on a kernel-heavy one, while not capping costs 1.3% at worst. `auto` is left +as the core count; `resolve_batch_threads` in `plan.py` carries the numbers. + +**Final result:** 26.33s serial → 24.71s at 6 threads on the 20-scenario benchmark, **6.1%**, on top +of the 35% the batching work delivered. All 20 scenarios byte-identical at every thread count, in +both the normal and the kernel-heavy regime. + +## What the tests found + +Every new test was mutation-checked, and two were not good enough on the first attempt: + +- The **fork** test earns its place. With the `pthread_atfork` handler removed the forked child + deadlocks on workers that no longer exist; the test kills it on a deadline and reports the timeout + rather than hanging the suite. +- The **concurrency** test initially did not bite. With the dispatch mutex deleted, 25 rounds over 4 + lanes passed happily — the GIL keeps two callers from overlapping often enough for the race to + land. At 400 rounds over 8 lanes it deadlocks within seconds. The round count is therefore + load-bearing and the test says so. +- The **scratch cross-context** test cannot fail before its change, since without reuse there is + nothing to leak; it was verified by relaxing `build_window_membership`'s `assign` to skip shrinking, + which makes it fail immediately.