Skip to content

Commit 9bf0549

Browse files
fix(kernel): key the window bounds cache on the window count as well as identity
Addresses two of the three review comments on this PR. A window list that grows or shrinks in place keeps its id(), so identity alone was not enough to decide a cache hit. run_prediction_kernel passes n_charge/n_export from len(window_list) alongside the cached arrays, so a stale shorter array would be read past its end by the C kernel - a memory-safety failure rather than merely a wrong plan. The count is now part of the hit condition. Nothing on the planning path resizes a window list today, so this guards the class rather than fixing a live defect. The test proves it would have bitten: before the change, appending to a cached list left a one-entry bounds array against two windows, and popping left window_bound_tuple returning the longer tuple. Also corrects the correctness note, which pointed at Plan.set_window_start() and Prediction.set_window_start(). Neither exists - they are module functions here - and after the rebase the prediction path does not use them at all, since _prepare_export applies a trial start copy-on-write to a window dict and list of its own. The third comment, to build the bound tuple from a generator rather than a list comprehension, is not taken: measured over 50k calls at 200 windows, the generator is 19.3% slower (307ms against 366ms), because it pays per-item interpreter overhead the specialised list comprehension avoids. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 4a0f828 commit 9bf0549

2 files changed

Lines changed: 39 additions & 5 deletions

File tree

apps/predbat/prediction_kernel.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -398,9 +398,10 @@ def int32_array(values):
398398
# time in a plan. They are cached here, keyed on the identity of the window list.
399399
#
400400
# CORRECTNESS: the cache is only sound while no window's start or end changes underneath it. Every
401-
# such mutation must call invalidate_window_cache() - route them through
402-
# Plan.set_window_start()/Prediction.set_window_start() rather than assigning window["start"]
403-
# directly. run_window_cache_tests replays a full plan with VALIDATE_WINDOW_CACHE on, which
401+
# such mutation must call invalidate_window_cache() - route them through the set_window_start() and
402+
# set_window_end() helpers below rather than assigning window["start"] or window["end"] directly.
403+
# The prediction path does not need them: _prepare_export applies a trial start copy-on-write, to a
404+
# window dict and list of its own, so nothing the cache has seen is touched. run_window_cache_tests replays a full plan with VALIDATE_WINDOW_CACHE on, which
404405
# re-derives the bounds on every hit and fails on any stale entry, so a missed invalidation is a
405406
# test failure rather than a silently wrong plan.
406407
#
@@ -457,13 +458,19 @@ def _window_cache_entry(window_list):
457458
"""
458459
key = id(window_list)
459460
entry = _WINDOW_BOUNDS_CACHE.get(key)
460-
if entry is not None and entry[0] is window_list:
461+
# The window count is part of the hit condition, not just the identity: a list that grows or
462+
# shrinks in place keeps its id(), and run_prediction_kernel passes n_charge/n_export from
463+
# len(window_list) alongside these arrays. A stale shorter array would then be read past its end
464+
# by the C kernel - a memory-safety failure rather than merely a wrong plan. Bare start/end
465+
# writes are handled by set_window_start/set_window_end instead; nothing on the planning path
466+
# resizes a window list today, so this is a guard against the class rather than a live fix.
467+
if entry is not None and entry[0] is window_list and entry[4] == len(window_list):
461468
if VALIDATE_WINDOW_CACHE:
462469
_validate_entry(window_list, entry)
463470
return entry
464471
if len(_WINDOW_BOUNDS_CACHE) >= WINDOW_CACHE_MAX:
465472
_WINDOW_BOUNDS_CACHE.clear()
466-
entry = [window_list, None, None, None]
473+
entry = [window_list, None, None, None, len(window_list)]
467474
_WINDOW_BOUNDS_CACHE[key] = entry
468475
return entry
469476

apps/predbat/tests/test_window.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,33 @@ def run_window_cache_tests(my_predbat):
457457
print("**** Running window cache tests ****")
458458
failed = False
459459

460+
# A window list that grows or shrinks in place keeps its id(), so the entry must also check the
461+
# length. Otherwise run_prediction_kernel passes n_charge = len(window_list) alongside the older,
462+
# shorter cached arrays and the C kernel reads past their end - a memory-safety failure rather
463+
# than merely a wrong plan, which is why this is guarded and not just documented.
464+
prediction_kernel.invalidate_window_cache()
465+
growing = [{"start": 0, "end": 30, "average": 5.0}]
466+
starts_before, ends_before = prediction_kernel.window_bound_arrays(growing)
467+
if len(starts_before) != 1:
468+
print("ERROR: expected a single-entry bounds array, got {}".format(len(starts_before)))
469+
failed = True
470+
growing.append({"start": 60, "end": 120, "average": 6.0})
471+
starts_after, ends_after = prediction_kernel.window_bound_arrays(growing)
472+
if len(starts_after) != len(growing) or len(ends_after) != len(growing):
473+
print("ERROR: bounds arrays are stale after the window list grew - len {} against {} windows".format(len(starts_after), len(growing)))
474+
failed = True
475+
elif list(starts_after) != [0, 60] or list(ends_after) != [30, 120]:
476+
print("ERROR: bounds arrays wrong after growth: {} / {}".format(list(starts_after), list(ends_after)))
477+
failed = True
478+
# The tuple used for the prediction cache key has to follow the same rule
479+
prediction_kernel.invalidate_window_cache()
480+
shrinking = [{"start": 0, "end": 30, "average": 5.0}, {"start": 60, "end": 120, "average": 6.0}]
481+
prediction_kernel.window_bound_tuple(shrinking)
482+
shrinking.pop()
483+
if prediction_kernel.window_bound_tuple(shrinking) != ((0, 30),):
484+
print("ERROR: window_bound_tuple is stale after the window list shrank: {}".format(prediction_kernel.window_bound_tuple(shrinking)))
485+
failed = True
486+
460487
# A repeated lookup on an unchanged list returns the identical cached objects
461488
prediction_kernel.invalidate_window_cache()
462489
windows = [{"start": 0, "end": 30, "average": 5.0}, {"start": 60, "end": 120, "average": 6.0}]

0 commit comments

Comments
 (0)