Performance improvements, better readme and complete python bindings

tests/CMakeLists.txt

@@ -33,6 +33,8 @@ add_executable(kpn_tests
     test_network.cpp
     test_static_network.cpp
     test_shared_resource.cpp
+    test_pool_node.cpp
+    test_scheduler.cpp
 )
 
 target_link_libraries(kpn_tests PRIVATE

tests/test_channel.cpp

@@ -1,6 +1,8 @@
 #include <catch2/catch_test_macros.hpp>
+#include <catch2/catch_approx.hpp>
 #include <kpn/channel.hpp>
 #include <thread>
+#include <vector>
 
 using namespace kpn;
 
@@ -70,13 +72,21 @@ TEST_CASE("push to disabled channel is silently dropped", "[channel]") {
     REQUIRE(ch.size() == 0);
 }
 
-TEST_CASE("disable clears existing queue contents", "[channel]") {
+TEST_CASE("disable stops accepting and unblocks pop", "[channel]") {
+    // With the SPSC lock-free ring, disable() does not drain the ring immediately;
+    // items are freed when the Channel is destroyed. What it must do is:
+    //  1. reject further pushes (drops silently)
+    //  2. unblock any waiting pop() (throws ChannelClosedError)
     Channel<int> ch(5);
     ch.push(1);
     ch.push(2);
     REQUIRE(ch.size() == 2);
     ch.disable();
-    REQUIRE(ch.size() == 0);
+    // Further pushes are dropped
+    ch.push(3);
+    REQUIRE(ch.size() <= 2);
+    // pop() must throw even though items remain in the ring
+    REQUIRE_THROWS_AS(ch.pop(), ChannelClosedError);
 }
 
 TEST_CASE("enable re-accepts pushes after disable", "[channel]") {
@@ -97,3 +107,71 @@ TEST_CASE("large type stored as shared_ptr — no copy on pop", "[channel]") {
     auto out = ch.pop();
     REQUIRE(out.tag == 123);
 }
+
+// ── ChannelDataSize / bytes_pushed tests ─────────────────────────────────────
+
+TEST_CASE("bytes_pushed uses sizeof(T) by default for POD type", "[channel][bandwidth]") {
+    Channel<int> ch(10);
+    ch.push(1);
+    ch.push(2);
+    ch.push(3);
+    ch.pop(); ch.pop(); ch.pop();
+
+    auto snap = ch.snapshot("test");
+    REQUIRE(snap.pushes == 3);
+    REQUIRE(snap.bytes_pushed == 3 * sizeof(int));
+}
+
+TEST_CASE("bytes_pushed uses sizeof(T) by default for large struct", "[channel][bandwidth]") {
+    struct Blob { char data[256]; };
+    Channel<Blob> ch(10);
+    ch.push(Blob{});
+    ch.push(Blob{});
+
+    auto snap = ch.snapshot("test");
+    REQUIRE(snap.bytes_pushed == 2 * sizeof(Blob));
+}
+
+// A fake heap-owning type whose logical payload size differs from sizeof.
+struct FakeFrame {
+    std::vector<uint8_t> pixels;
+};
+
+// Specialise ChannelDataSize so the channel counts actual pixel bytes.
+template<>
+struct kpn::ChannelDataSize<FakeFrame> {
+    static std::size_t bytes(const FakeFrame& f) { return f.pixels.size(); }
+};
+
+TEST_CASE("bytes_pushed uses ChannelDataSize specialisation for heap-owning type", "[channel][bandwidth]") {
+    Channel<FakeFrame> ch(10);
+    ch.push(FakeFrame{std::vector<uint8_t>(1000)});
+    ch.push(FakeFrame{std::vector<uint8_t>(2000)});
+
+    auto snap = ch.snapshot("test");
+    REQUIRE(snap.pushes == 2);
+    REQUIRE(snap.bytes_pushed == 3000);
+    // item_bytes is still sizeof(FakeFrame) — the struct header
+    REQUIRE(snap.item_bytes == sizeof(FakeFrame));
+}
+
+TEST_CASE("bandwidth_mbs is non-zero and correct for heap-owning type", "[channel][bandwidth]") {
+    Channel<FakeFrame> ch(10);
+    // Push 10 frames of 1 MB each
+    for (int i = 0; i < 10; ++i)
+        ch.push(FakeFrame{std::vector<uint8_t>(1'000'000)});
+
+    auto snap = ch.snapshot("test");
+    // 10 MB over 1 second => 10.0 MB/s
+    REQUIRE(snap.bandwidth_mbs(1.0) == Catch::Approx(10.0).epsilon(1e-6));
+    // Without the fix (using sizeof), this would have been ~40 bytes/s ≈ 0.00004 MB/s.
+    REQUIRE(snap.bandwidth_mbs(1.0) > 1.0);
+}
+
+TEST_CASE("bandwidth_mbs returns 0 when elapsed_s is zero or negative", "[channel][bandwidth]") {
+    Channel<int> ch(5);
+    ch.push(42);
+    auto snap = ch.snapshot("test");
+    REQUIRE(snap.bandwidth_mbs(0.0) == 0.0);
+    REQUIRE(snap.bandwidth_mbs(-1.0) == 0.0);
+}

tests/test_pool_node.cpp

@@ -0,0 +1,241 @@
+#include <catch2/catch_test_macros.hpp>
+#include <kpn/scheduler.hpp>
+#include <kpn/pool_node.hpp>
+#include <kpn/interrupt_node.hpp>
+#include <atomic>
+#include <chrono>
+#include <thread>
+
+using namespace kpn;
+
+static int double_it(int x) { return x * 2; }
+static std::tuple<int, float> split_it(int x) { return {x, float(x) * 0.5f}; }
+static void consume_it(int x) { (void)x; }
+
+// ── ThreadPool ────────────────────────────────────────────────────────────────
+
+TEST_CASE("thread pool starts and stops cleanly", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+    pool.stop();
+}
+
+TEST_CASE("thread pool executes submitted tasks", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    for (int i = 0; i < 10; ++i)
+        pool.submit([&] { counter.fetch_add(1); });
+
+    pool.drain();
+    REQUIRE(counter.load() == 10);
+    pool.stop();
+}
+
+TEST_CASE("thread pool drain waits for all tasks", "[scheduler]") {
+    ThreadPool pool(1);
+    pool.start();
+
+    std::atomic<bool> done{false};
+    pool.submit([&] {
+        std::this_thread::sleep_for(std::chrono::milliseconds(20));
+        done.store(true);
+    });
+    pool.drain();
+    REQUIRE(done.load());
+    pool.stop();
+}
+
+TEST_CASE("thread pool priority: higher priority tasks run first", "[scheduler]") {
+    ThreadPool pool(1);  // single thread so order is deterministic
+    pool.start();
+
+    // Submit a task that blocks the worker, then queue two tasks with
+    // different priorities. When the blocker finishes, the high-priority
+    // task should run before the low-priority one.
+    std::vector<int> order;
+    std::mutex order_mutex;
+
+    std::atomic<bool> blocker_done{false};
+    pool.submit([&] {
+        std::this_thread::sleep_for(std::chrono::milliseconds(30));
+        blocker_done.store(true);
+    }, 0.5f);
+
+    // Wait until blocker is running, then enqueue the two ordered tasks.
+    while (!blocker_done.load()) std::this_thread::sleep_for(std::chrono::milliseconds(1));
+
+    pool.submit([&] { std::lock_guard g(order_mutex); order.push_back(1); }, 0.1f);
+    pool.submit([&] { std::lock_guard g(order_mutex); order.push_back(2); }, 0.9f);
+
+    pool.drain();
+    REQUIRE(order == std::vector<int>{2, 1});
+    pool.stop();
+}
+
+// ── PoolNode ──────────────────────────────────────────────────────────────────
+
+TEST_CASE("pool node input/output counts", "[pool_node]") {
+    STATIC_REQUIRE(PoolNode<double_it>::input_count  == 1);
+    STATIC_REQUIRE(PoolNode<double_it>::output_count == 1);
+    STATIC_REQUIRE(PoolNode<split_it>::output_count  == 2);
+    STATIC_REQUIRE(PoolNode<consume_it>::output_count == 0);
+}
+
+TEST_CASE("pool node processes items end-to-end", "[pool_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    auto node = make_pool_node<double_it>(pool);
+    Channel<int> out_ch(10);
+    node.set_output_channel<0>(&out_ch);
+    node.start();
+
+    node.input_channel<0>().push(21);
+    int result = out_ch.pop();
+
+    node.stop();
+    pool->stop();
+
+    REQUIRE(result == 42);
+}
+
+TEST_CASE("pool node processes multiple items in order", "[pool_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    auto node = make_pool_node<double_it>(pool, 20);  // capacity 20
+    Channel<int> out_ch(20);
+    node.set_output_channel<0>(&out_ch);
+    node.start();
+
+    constexpr int N = 10;
+    for (int i = 0; i < N; ++i)
+        node.input_channel<0>().push(i);
+
+    std::vector<int> results;
+    for (int i = 0; i < N; ++i)
+        results.push_back(out_ch.pop());
+
+    node.stop();
+    pool->stop();
+
+    REQUIRE(results.size() == N);
+    for (int i = 0; i < N; ++i)
+        REQUIRE(results[i] == i * 2);
+}
+
+TEST_CASE("pool node stop is clean with no deadlock", "[pool_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    auto node = make_pool_node<double_it>(pool);
+    node.start();
+    // Node is idle (no input pushed) — stop must return without deadlock.
+    node.stop();
+    REQUIRE_FALSE(node.running());
+
+    pool->stop();
+}
+
+TEST_CASE("pool node two-stage pipeline produces correct count", "[pool_node]") {
+    auto pool = std::make_shared<ThreadPool>(4);
+    pool->start();
+
+    auto src       = make_pool_node<double_it>(pool);
+    auto transform = make_pool_node<double_it>(pool);
+    Channel<int> out_ch(20);
+
+    // Wire src output → transform input channel, transform output → out_ch.
+    src.set_output_channel<0>(&transform.input_channel<0>());
+    transform.set_output_channel<0>(&out_ch);
+
+    src.start();
+    transform.start();
+
+    constexpr int N = 5;
+    for (int i = 1; i <= N; ++i)
+        src.input_channel<0>().push(i);
+
+    std::vector<int> results;
+    for (int i = 0; i < N; ++i)
+        results.push_back(out_ch.pop());
+
+    // Stop nodes before they (and their channels) go out of scope.
+    src.stop();
+    transform.stop();
+    pool->stop();
+
+    REQUIRE(results.size() == static_cast<std::size_t>(N));
+    for (int i = 0; i < N; ++i)
+        REQUIRE(results[i] == (i + 1) * 4);  // double_it twice
+}
+
+// ── InterruptNode ─────────────────────────────────────────────────────────────
+
+namespace {
+static std::atomic<int> g_interrupt_counter{0};
+static int interrupt_produce() { return g_interrupt_counter.fetch_add(1); }
+} // namespace
+
+TEST_CASE("interrupt node fires on each trigger", "[interrupt_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    g_interrupt_counter.store(0);
+    auto node = make_interrupt_node<interrupt_produce>(pool, out<>{});
+    Channel<int> out_ch(20);
+    node.set_output_channel<0>(&out_ch);
+    node.start();
+
+    auto trigger = node.get_trigger();
+    constexpr int N = 5;
+    for (int i = 0; i < N; ++i) trigger();
+
+    std::vector<int> results;
+    for (int i = 0; i < N; ++i)
+        results.push_back(out_ch.pop());
+
+    node.stop();
+    pool->stop();
+
+    REQUIRE(results.size() == static_cast<std::size_t>(N));
+}
+
+TEST_CASE("interrupt node does not fire without trigger", "[interrupt_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    g_interrupt_counter.store(0);
+    auto node = make_interrupt_node<interrupt_produce>(pool, out<>{});
+    Channel<int> out_ch(5);
+    node.set_output_channel<0>(&out_ch);
+    node.start();
+
+    std::this_thread::sleep_for(std::chrono::milliseconds(30));
+    // No trigger fired — output channel should be empty.
+    REQUIRE(out_ch.approx_size() == 0);
+
+    node.stop();
+    pool->stop();
+}
+
+TEST_CASE("interrupt node: trigger after stop is ignored", "[interrupt_node]") {
+    auto pool = std::make_shared<ThreadPool>(2);
+    pool->start();
+
+    g_interrupt_counter.store(0);
+    auto node = make_interrupt_node<interrupt_produce>(pool, out<>{});
+    Channel<int> out_ch(5);
+    node.set_output_channel<0>(&out_ch);
+    node.start();
+
+    auto trigger = node.get_trigger();
+    node.stop();
+    trigger();  // should be a no-op
+
+    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    REQUIRE(out_ch.approx_size() == 0);
+    pool->stop();
+}

tests/test_scheduler.cpp

@@ -0,0 +1,229 @@
+#include <catch2/catch_test_macros.hpp>
+#include <kpn/scheduler.hpp>
+
+#include <atomic>
+#include <chrono>
+#include <thread>
+#include <vector>
+#include <mutex>
+
+using namespace kpn;
+using namespace std::chrono_literals;
+
+// ── basic execution ───────────────────────────────────────────────────────────
+
+TEST_CASE("scheduler runs submitted tasks", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    for (int i = 0; i < 100; ++i)
+        pool.submit([&counter]{ counter.fetch_add(1, std::memory_order_relaxed); });
+
+    pool.drain();
+    REQUIRE(counter.load() == 100);
+    pool.stop();
+}
+
+TEST_CASE("scheduler single thread executes all tasks", "[scheduler]") {
+    ThreadPool pool(1);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    for (int i = 0; i < 50; ++i)
+        pool.submit([&counter]{ counter.fetch_add(1, std::memory_order_relaxed); });
+
+    pool.drain();
+    REQUIRE(counter.load() == 50);
+    pool.stop();
+}
+
+// ── drain ─────────────────────────────────────────────────────────────────────
+
+TEST_CASE("drain returns immediately when pool is idle", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+    pool.drain();   // nothing submitted — should return immediately
+    pool.stop();
+}
+
+TEST_CASE("drain waits for all tasks to complete", "[scheduler]") {
+    ThreadPool pool(4);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    constexpr int N = 200;
+
+    for (int i = 0; i < N; ++i) {
+        pool.submit([&counter]{
+            std::this_thread::sleep_for(1ms);
+            counter.fetch_add(1, std::memory_order_relaxed);
+        });
+    }
+
+    pool.drain();
+    REQUIRE(counter.load() == N);
+    pool.stop();
+}
+
+TEST_CASE("drain is safe to call multiple times", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    pool.submit([&counter]{ counter.fetch_add(1, std::memory_order_relaxed); });
+    pool.drain();
+    REQUIRE(counter.load() == 1);
+
+    pool.submit([&counter]{ counter.fetch_add(1, std::memory_order_relaxed); });
+    pool.drain();
+    REQUIRE(counter.load() == 2);
+
+    pool.stop();
+}
+
+// ── priority ordering ─────────────────────────────────────────────────────────
+
+TEST_CASE("higher priority tasks run before lower priority on single thread", "[scheduler]") {
+    // Single thread guarantees serial execution — we can observe order.
+    ThreadPool pool(1);
+    pool.start();
+
+    // Pause the worker so we can fill the queue before it drains.
+    std::mutex gate;
+    gate.lock();
+    pool.submit([&gate]{ std::lock_guard lg(gate); }); // blocks worker
+
+    std::vector<float> order;
+    std::mutex order_mx;
+
+    for (float p : {0.1f, 0.9f, 0.5f, 0.8f, 0.2f}) {
+        pool.submit([p, &order, &order_mx]{
+            std::lock_guard lg(order_mx);
+            order.push_back(p);
+        }, p);
+    }
+
+    gate.unlock();   // release the blocking task
+    pool.drain();
+    pool.stop();
+
+    // order should be descending by priority
+    REQUIRE(order.size() == 5);
+    for (std::size_t i = 1; i < order.size(); ++i)
+        REQUIRE(order[i - 1] >= order[i]);
+}
+
+TEST_CASE("equal priority tasks execute in FIFO order on single thread", "[scheduler]") {
+    ThreadPool pool(1);
+    pool.start();
+
+    std::mutex gate;
+    gate.lock();
+    pool.submit([&gate]{ std::lock_guard lg(gate); });
+
+    std::vector<int> order;
+    std::mutex order_mx;
+
+    for (int i = 0; i < 5; ++i) {
+        pool.submit([i, &order, &order_mx]{
+            std::lock_guard lg(order_mx);
+            order.push_back(i);
+        }, 0.5f);   // all same priority
+    }
+
+    gate.unlock();
+    pool.drain();
+    pool.stop();
+
+    REQUIRE(order == std::vector<int>{0, 1, 2, 3, 4});
+}
+
+// ── total_ / active_ accounting ───────────────────────────────────────────────
+
+TEST_CASE("snapshot queue depth and active counts are consistent", "[scheduler]") {
+    ThreadPool pool(2);
+    pool.start();
+
+    // While tasks are running, active should be > 0 and total >= active.
+    std::atomic<bool> running{false};
+    std::mutex gate;
+    gate.lock();
+
+    for (int i = 0; i < 4; ++i) {
+        pool.submit([&gate, &running]{
+            running.store(true, std::memory_order_relaxed);
+            std::lock_guard lg(gate);
+        });
+    }
+
+    // Spin until at least one task has started
+    while (!running.load(std::memory_order_relaxed))
+        std::this_thread::yield();
+
+    auto snap = pool.snapshot("test");
+    REQUIRE(snap.active_count > 0);
+    REQUIRE(snap.queue_depth + snap.active_count > 0);
+
+    gate.unlock();
+    pool.drain();
+
+    auto snap2 = pool.snapshot("test");
+    REQUIRE(snap2.active_count == 0);
+    REQUIRE(snap2.queue_depth  == 0);
+
+    pool.stop();
+}
+
+TEST_CASE("submitted and completed counters are accurate", "[scheduler]") {
+    ThreadPool pool(3);
+    pool.start();
+
+    constexpr int N = 60;
+    for (int i = 0; i < N; ++i)
+        pool.submit([]{ std::this_thread::yield(); });
+
+    pool.drain();
+    auto snap = pool.snapshot("test");
+    REQUIRE(snap.tasks_submitted == static_cast<uint64_t>(N));
+    REQUIRE(snap.tasks_completed == static_cast<uint64_t>(N));
+
+    pool.stop();
+}
+
+// ── work stealing ─────────────────────────────────────────────────────────────
+
+TEST_CASE("work stealing: all tasks complete with uneven initial distribution", "[scheduler]") {
+    // 4-thread pool. Submit a burst to ensure some threads start empty and must steal.
+    ThreadPool pool(4);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    constexpr int N = 400;
+
+    for (int i = 0; i < N; ++i)
+        pool.submit([&counter]{
+            std::this_thread::sleep_for(100us);
+            counter.fetch_add(1, std::memory_order_relaxed);
+        });
+
+    pool.drain();
+    REQUIRE(counter.load() == N);
+    pool.stop();
+}
+
+TEST_CASE("work stealing: tasks complete with more threads than initial queue targets", "[scheduler]") {
+    // With round-robin, some threads may get no tasks initially and must steal.
+    constexpr std::size_t THREADS = 8;
+    ThreadPool pool(THREADS);
+    pool.start();
+
+    std::atomic<int> counter{0};
+    // Submit fewer tasks than threads so most threads must steal
+    for (int i = 0; i < 4; ++i)
+        pool.submit([&counter]{ counter.fetch_add(1, std::memory_order_relaxed); });
+
+    pool.drain();
+    REQUIRE(counter.load() == 4);
+    pool.stop();
+}