From 4501327534a0bc7a66e3691cb65246e5404e84f0 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Thu, 13 Aug 2026 13:11:25 +0100 Subject: [PATCH 1/2] perf(prediction): build the kernel's ctypes arrays via array.array The six per-simulation ctypes buffers were built as (ctypes.c_double * n)(*values), which unpacks the list into positional arguments - several times slower than copying from an array.array of the same type. Marshalling was the largest remaining cost in the planner after #4505, #4507 and #4508: 8.1s of a 13.9s profiled plan, more than everything else combined. Measured on the shapes the planner actually passes, building the charge window geometry drops from 35.0us to 12.7us per call. Content-keyed memoisation of the geometry arrays was measured as an alternative (10.8us) and rejected: it is only marginally ahead, because the key still has to be built and hashed, and it would have to stay correct across the passes that mutate window bounds in place. Reusing the soc_out buffer was also measured and is not worth it at 0.15us per allocation. from_buffer returns a view over the array.array rather than a copy, so the backing object has to outlive the kernel call. ctypes keeps it alive through the view's _objects; a new test asserts that rather than assuming it, since the failure mode is reading freed memory silently. The pool workers are separate forked processes so no buffer is shared between them, which was verified by running the same plan single-process and pooled and confirming identical results (not added as a test - it needs two full plan runs). Benchmark: worst scenario 11.03s -> 8.39s, mean optimise time across the 20 scenarios 3.717s -> 1.961s (1.9x), with plan metric and cost identical on all 20. Co-Authored-By: Claude Fable 5 --- apps/predbat/prediction_kernel.py | 19 ++++++++--- apps/predbat/tests/test_kernel_parity.py | 37 ++++++++++++++++++++- coverage/cases/random_results.json | 42 ++++++++++++------------ 3 files changed, 72 insertions(+), 26 deletions(-) diff --git a/apps/predbat/prediction_kernel.py b/apps/predbat/prediction_kernel.py index 94a290241..1e8471f1c 100644 --- a/apps/predbat/prediction_kernel.py +++ b/apps/predbat/prediction_kernel.py @@ -20,6 +20,7 @@ rejected at load time rather than producing divergent results. """ +import array import ctypes import os import platform @@ -261,13 +262,23 @@ def load_kernel(log=None): def double_array(values): - """Create a ctypes double array from a Python list""" - return (ctypes.c_double * len(values))(*values) + """Create a ctypes double array from a Python list. + + Built via array.array rather than (ctypes.c_double * n)(*values): the latter unpacks the list as + positional arguments and is several times slower, which matters because these are rebuilt on + every simulation. from_buffer returns a view over the array.array, and ctypes keeps the backing + object alive through the view's _objects, so the buffer cannot be collected while the kernel is + using it. Each pool worker is a separate process (multiprocessing with fork), so no buffer is + ever shared between workers. + """ + backing = array.array("d", values) + return (ctypes.c_double * len(backing)).from_buffer(backing) def int32_array(values): - """Create a ctypes int32 array from a Python list""" - return (ctypes.c_int32 * len(values))(*values) + """Create a ctypes int32 array from a Python list - see double_array for why array.array is used""" + backing = array.array("i", values) + return (ctypes.c_int32 * len(backing)).from_buffer(backing) def kernel_context_free(handle): diff --git a/apps/predbat/tests/test_kernel_parity.py b/apps/predbat/tests/test_kernel_parity.py index 0187943c0..433ecf61b 100644 --- a/apps/predbat/tests/test_kernel_parity.py +++ b/apps/predbat/tests/test_kernel_parity.py @@ -21,6 +21,7 @@ """ import copy +import gc import os import random import subprocess @@ -380,6 +381,39 @@ def dual_run(name, my_predbat, pv_step, pv10_step, load_step, load10_step, charg return failed +def run_marshalling_tests(): + """Check the ctypes buffer helpers, returns True on failure. + + double_array/int32_array build their buffers with from_buffer, which returns a view over an + array.array rather than a copy. If ctypes did not keep the backing object alive the kernel would + read freed memory - silently, and only sometimes - so that guarantee is asserted here rather than + assumed, along with the values surviving the round trip. + """ + print("**** Running kernel marshalling tests ****") + failed = False + + for name, builder, values in (("double_array", prediction_kernel.double_array, [0.0, -1.5, 3.25, 1e6]), ("int32_array", prediction_kernel.int32_array, [0, -7, 42, 100000])): + # Build from a temporary so the source list/array is unreferenced by the time it is read + buffer = builder(list(values)) + gc.collect() + got = [buffer[i] for i in range(len(values))] + if got != values: + print("ERROR: {} round trip expected {} but got {}".format(name, values, got)) + failed = True + if buffer._objects is None: + print("ERROR: {} did not retain its backing buffer - the kernel could read freed memory".format(name)) + failed = True + + empty = prediction_kernel.int32_array([]) + if len(empty) != 0: + print("ERROR: int32_array([]) should be empty, got length {}".format(len(empty))) + failed = True + + if not failed: + print("PASS") + return failed + + def run_edge_case_tests(my_predbat): """Deterministic scenarios pinning each kernel branch, returns True on failure""" failed = False @@ -840,7 +874,8 @@ def run_kernel_parity_tests(my_predbat): state = snapshot_scenario_state(my_predbat) try: - failed = run_edge_case_tests(my_predbat) + failed = run_marshalling_tests() + failed |= run_edge_case_tests(my_predbat) if not failed: failed |= run_random_sweep_tests(my_predbat) if not failed: diff --git a/coverage/cases/random_results.json b/coverage/cases/random_results.json index 1e9db924c..a8df6b236 100644 --- a/coverage/cases/random_results.json +++ b/coverage/cases/random_results.json @@ -2,7 +2,7 @@ "run_info": { "template_yaml": "cases/predbat_debug_agile1.yaml", "scenarios_file": "cases/random_scenarios.yaml", - "timestamp": "2026-08-13T10:34:13.978454+00:00" + "timestamp": "2026-08-13T11:57:01.949843+00:00" }, "results": [ { @@ -17,7 +17,7 @@ "soc_final": 6.8616, "battery_cycles": 21.0198, "carbon_g": 12959.33, - "runtime_s": 3.993, + "runtime_s": 2.1, "failed": false, "error": null, "end_record": 1440 @@ -34,7 +34,7 @@ "soc_final": 4.8, "battery_cycles": 10.5525, "carbon_g": 4613.47, - "runtime_s": 3.174, + "runtime_s": 2.772, "failed": false, "error": null, "end_record": 1440 @@ -51,7 +51,7 @@ "soc_final": 2.2382, "battery_cycles": 5.8265, "carbon_g": 9017.53, - "runtime_s": 3.094, + "runtime_s": 1.561, "failed": false, "error": null, "end_record": 1440 @@ -68,7 +68,7 @@ "soc_final": 2.0559, "battery_cycles": 9.7908, "carbon_g": 9673.45, - "runtime_s": 0.236, + "runtime_s": 0.211, "failed": false, "error": null, "end_record": 1440 @@ -85,7 +85,7 @@ "soc_final": 4.8, "battery_cycles": 7.3257, "carbon_g": 15944.91, - "runtime_s": 0.423, + "runtime_s": 0.364, "failed": false, "error": null, "end_record": 1440 @@ -102,7 +102,7 @@ "soc_final": 2.3066, "battery_cycles": 34.749, "carbon_g": 884.71, - "runtime_s": 0.433, + "runtime_s": 0.39, "failed": false, "error": null, "end_record": 1440 @@ -119,7 +119,7 @@ "soc_final": 0.38, "battery_cycles": 38.4449, "carbon_g": 8065.01, - "runtime_s": 18.238, + "runtime_s": 8.31, "failed": false, "error": null, "end_record": 1440 @@ -136,7 +136,7 @@ "soc_final": 0.38, "battery_cycles": 14.2039, "carbon_g": 18350.09, - "runtime_s": 4.406, + "runtime_s": 3.376, "failed": false, "error": null, "end_record": 1440 @@ -153,7 +153,7 @@ "soc_final": 0.38, "battery_cycles": 10.6638, "carbon_g": 12867.91, - "runtime_s": 0.417, + "runtime_s": 0.361, "failed": false, "error": null, "end_record": 1440 @@ -170,7 +170,7 @@ "soc_final": 9.152, "battery_cycles": 25.215, "carbon_g": 4663.11, - "runtime_s": 0.566, + "runtime_s": 0.512, "failed": false, "error": null, "end_record": 1440 @@ -187,7 +187,7 @@ "soc_final": 1.0713, "battery_cycles": 25.7368, "carbon_g": 7751.35, - "runtime_s": 2.156, + "runtime_s": 1.404, "failed": false, "error": null, "end_record": 1440 @@ -204,7 +204,7 @@ "soc_final": 0.38, "battery_cycles": 6.174, "carbon_g": 13156.97, - "runtime_s": 1.181, + "runtime_s": 0.768, "failed": false, "error": null, "end_record": 1440 @@ -221,7 +221,7 @@ "soc_final": 0.38, "battery_cycles": 28.7593, "carbon_g": 15144.61, - "runtime_s": 3.545, + "runtime_s": 2.101, "failed": false, "error": null, "end_record": 1440 @@ -238,7 +238,7 @@ "soc_final": 6.2665, "battery_cycles": 9.68, "carbon_g": 12838.7, - "runtime_s": 0.499, + "runtime_s": 0.433, "failed": false, "error": null, "end_record": 1440 @@ -255,7 +255,7 @@ "soc_final": 1.5389, "battery_cycles": 5.6825, "carbon_g": 16611.75, - "runtime_s": 0.477, + "runtime_s": 0.415, "failed": false, "error": null, "end_record": 1440 @@ -272,7 +272,7 @@ "soc_final": 4.3726, "battery_cycles": 19.543, "carbon_g": 16613.71, - "runtime_s": 4.61, + "runtime_s": 2.725, "failed": false, "error": null, "end_record": 1440 @@ -289,7 +289,7 @@ "soc_final": 0.38, "battery_cycles": 4.9586, "carbon_g": 15848.43, - "runtime_s": 0.297, + "runtime_s": 0.255, "failed": false, "error": null, "end_record": 1440 @@ -306,7 +306,7 @@ "soc_final": 0.38, "battery_cycles": 25.8549, "carbon_g": 153.34, - "runtime_s": 5.097, + "runtime_s": 1.887, "failed": false, "error": null, "end_record": 1440 @@ -323,7 +323,7 @@ "soc_final": 0.5251, "battery_cycles": 9.8445, "carbon_g": 11504.26, - "runtime_s": 0.212, + "runtime_s": 0.19, "failed": false, "error": null, "end_record": 1440 @@ -340,7 +340,7 @@ "soc_final": 19.6907, "battery_cycles": 60.7987, "carbon_g": 18744.27, - "runtime_s": 21.279, + "runtime_s": 9.089, "failed": false, "error": null, "end_record": 1440 From e10031ca60184bbf39061ba00194d71f3abbabed Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Thu, 13 Aug 2026 13:32:19 +0100 Subject: [PATCH 2/2] fix(prediction): guard the ctypes array typecode against a width mismatch Review feedback on #4509. array.array's integer typecodes are C types, so 'i' is a C int - 32 bit everywhere predbat runs, but not guaranteed to be. The failure mode is not an exception: from_buffer only checks the buffer is large enough, so a wider backing type is accepted and the kernel reads interleaved garbage. Confirmed directly - building a c_int32 array over an array('d') backing returns [0, 1072693248, 0] rather than raising. The typecode is now chosen at import by matching itemsize against the ctypes element, falling back to the slower (ctypes.c_double * n)(*values) construction if nothing matches, so a platform with unusual widths loses the speed-up rather than silently corrupting the simulation inputs. Also from the review: the retention check now uses truthiness via getattr rather than "is not None", since an empty _objects means nothing is retained and is just as unsafe as the attribute being absent; and the empty-input case is asserted for double_array as well as int32_array. Mutation-testing the new guard turned up a bug in the test itself: it asserted _objects retention unconditionally, but the fallback path copies its values and so has nothing to retain. On any platform taking the fallback the suite would have failed spuriously. The assertion is now scoped to the from_buffer path. Verified by forcing each state: correct typecodes pass, no typecode (fallback) passes, and a deliberately wrong-width typecode is caught by both the itemsize check and the round-trip check. Co-Authored-By: Claude Fable 5 --- .cspell/custom-dictionary-workspace.txt | 3 +++ apps/predbat/prediction_kernel.py | 28 ++++++++++++++++++++++-- apps/predbat/tests/test_kernel_parity.py | 26 +++++++++++++++++----- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/.cspell/custom-dictionary-workspace.txt b/.cspell/custom-dictionary-workspace.txt index c21a27f55..927a8a965 100644 --- a/.cspell/custom-dictionary-workspace.txt +++ b/.cspell/custom-dictionary-workspace.txt @@ -234,6 +234,7 @@ invname isinstance isoformat isort +itemsize itemtype ivtime jedlix @@ -507,6 +508,8 @@ treforsiphone treforsouthwell tunables twinx +typecode +typecodes tzfile tzpath unconfigured diff --git a/apps/predbat/prediction_kernel.py b/apps/predbat/prediction_kernel.py index 1e8471f1c..b06339d63 100644 --- a/apps/predbat/prediction_kernel.py +++ b/apps/predbat/prediction_kernel.py @@ -261,6 +261,26 @@ def load_kernel(log=None): return KERNEL_LIB +def select_array_typecode(candidates, ctype): + """Pick an array.array typecode whose itemsize matches ctype exactly, or None if none does. + + array.array's integer typecodes are C types, so their widths are platform-defined - 'i' is a C + int, which is 32 bit everywhere predbat runs but is not guaranteed to be. Getting this wrong is + not a loud failure: from_buffer only checks the buffer is big enough, so a wider backing type is + accepted and the kernel silently reads interleaved garbage. Matching the size up front, and + falling back to the slower construction when nothing matches, keeps that impossible. + """ + size = ctypes.sizeof(ctype) + for typecode in candidates: + if array.array(typecode).itemsize == size: + return typecode + return None + + +DOUBLE_TYPECODE = select_array_typecode(("d", "f"), ctypes.c_double) +INT32_TYPECODE = select_array_typecode(("i", "l", "h"), ctypes.c_int32) + + def double_array(values): """Create a ctypes double array from a Python list. @@ -271,13 +291,17 @@ def double_array(values): using it. Each pool worker is a separate process (multiprocessing with fork), so no buffer is ever shared between workers. """ - backing = array.array("d", values) + if DOUBLE_TYPECODE is None: + return (ctypes.c_double * len(values))(*values) + backing = array.array(DOUBLE_TYPECODE, values) return (ctypes.c_double * len(backing)).from_buffer(backing) def int32_array(values): """Create a ctypes int32 array from a Python list - see double_array for why array.array is used""" - backing = array.array("i", values) + if INT32_TYPECODE is None: + return (ctypes.c_int32 * len(values))(*values) + backing = array.array(INT32_TYPECODE, values) return (ctypes.c_int32 * len(backing)).from_buffer(backing) diff --git a/apps/predbat/tests/test_kernel_parity.py b/apps/predbat/tests/test_kernel_parity.py index 433ecf61b..816c4c1c7 100644 --- a/apps/predbat/tests/test_kernel_parity.py +++ b/apps/predbat/tests/test_kernel_parity.py @@ -20,7 +20,9 @@ PREDBAT_KERNEL_REQUIRED=1 is set (CI) in which case it fails. """ +import array import copy +import ctypes import gc import os import random @@ -392,7 +394,15 @@ def run_marshalling_tests(): print("**** Running kernel marshalling tests ****") failed = False - for name, builder, values in (("double_array", prediction_kernel.double_array, [0.0, -1.5, 3.25, 1e6]), ("int32_array", prediction_kernel.int32_array, [0, -7, 42, 100000])): + # The typecode chosen for the backing array must match the ctypes element exactly. from_buffer + # only checks the buffer is large enough, so a wider backing type is accepted and then read as + # interleaved garbage - a silent corruption rather than an exception. + for name, typecode, ctype in (("DOUBLE_TYPECODE", prediction_kernel.DOUBLE_TYPECODE, ctypes.c_double), ("INT32_TYPECODE", prediction_kernel.INT32_TYPECODE, ctypes.c_int32)): + if typecode is not None and array.array(typecode).itemsize != ctypes.sizeof(ctype): + print("ERROR: {} is '{}' with itemsize {} but the ctypes element is {} bytes".format(name, typecode, array.array(typecode).itemsize, ctypes.sizeof(ctype))) + failed = True + + for name, builder, values, typecode in (("double_array", prediction_kernel.double_array, [0.0, -1.5, 3.25, 1e6], prediction_kernel.DOUBLE_TYPECODE), ("int32_array", prediction_kernel.int32_array, [0, -7, 42, 100000], prediction_kernel.INT32_TYPECODE)): # Build from a temporary so the source list/array is unreferenced by the time it is read buffer = builder(list(values)) gc.collect() @@ -400,14 +410,18 @@ def run_marshalling_tests(): if got != values: print("ERROR: {} round trip expected {} but got {}".format(name, values, got)) failed = True - if buffer._objects is None: + # Only the from_buffer path holds a view that needs its backing kept alive; the fallback + # copies the values, so it has nothing to retain and is safe without _objects. Truthiness + # rather than "is not None": an empty _objects would mean nothing is retained, which is just + # as unsafe as the attribute being absent. + if typecode is not None and not getattr(buffer, "_objects", None): print("ERROR: {} did not retain its backing buffer - the kernel could read freed memory".format(name)) failed = True - empty = prediction_kernel.int32_array([]) - if len(empty) != 0: - print("ERROR: int32_array([]) should be empty, got length {}".format(len(empty))) - failed = True + empty = builder([]) + if len(empty) != 0: + print("ERROR: {}([]) should be empty, got length {}".format(name, len(empty))) + failed = True if not failed: print("PASS")