fix(thread-pool): fix thread count vs binding constructor bug

THREAD_POOL_BEHAVIOR.md

@@ -0,0 +1,338 @@
+# ThreadPool Constructor and Configuration Behavior
+
+## Why this changed
+
+`ThreadPool` previously exposed both of these constructors:
+
+```cpp
+ThreadPool(uint32_t size, bool binding);
+ThreadPool(bool binding);
+```
+
+Calls such as `ThreadPool(1)` selected the exact `bool` overload instead of
+converting `1` to `uint32_t`. As a result, a caller intending to create one
+thread actually created `hardware_concurrency()` threads with CPU binding
+enabled. Any positive one-argument thread count had the same problem.
+
+The ambiguous overload was removed. Thread count and CPU binding must now be
+specified together:
+
+```cpp
+ThreadPool(uint32_t size, bool binding);
+```
+
+## Direct C++ API
+
+| Construction | Thread count | CPU binding |
+|---|---:|---|
+| `ThreadPool()` | `max(std::thread::hardware_concurrency(), 1)` | Disabled |
+| `ThreadPool(n, false)` | `n` | Disabled |
+| `ThreadPool(n, true)` | `n` | Enabled |
+
+The default constructor has not changed: it uses hardware concurrency, never
+zero, with binding disabled.
+
+The following one-argument forms are no longer supported:
+
+```cpp
+ThreadPool(n);
+ThreadPool(true);
+ThreadPool(false);
+```
+
+Use explicit replacements:
+
+```cpp
+// Old: ThreadPool(false)
+ThreadPool();
+
+// Old: ThreadPool(true)
+ThreadPool(std::max(std::thread::hardware_concurrency(), 1u), true);
+
+// Intended meaning of old ThreadPool(n)
+ThreadPool(n, false);  // or true when binding is required
+```
+
+This is a C++ source compatibility change. It intentionally turns ambiguous
+calls into compile errors instead of silently choosing the wrong behavior.
+
+CPU binding is implemented on Linux excluding Android. On unsupported
+platforms, the binding flag does not change thread affinity. Linux binding
+enumerates the calling thread's allowed CPU mask, so non-zero-based and
+restricted cpusets are honored. Affinity API failures are logged.
+
+## DB global query and optimize pools
+
+The DB pools do not use the direct `ThreadPool()` default. Their counts are
+initialized independently from `CgroupUtil::getCpuLimit()`:
+
+| Configuration | Default count | Default binding |
+|---|---:|---|
+| Query pool | Cgroup CPU limit | Enabled |
+| Optimize pool | Cgroup CPU limit | Enabled |
+
+Therefore:
+
+- Omitting a DB thread count does **not** mean one thread.
+- On an unrestricted host, the cgroup CPU limit will commonly equal available
+  CPU capacity.
+- In a CPU-limited container, the DB defaults may differ from
+  `std::thread::hardware_concurrency()`.
+- Cgroup v2 `cpu.max` values are parsed as `quota period`; `max period` is
+  treated as unlimited and falls back to the online CPU count.
+- Query and optimize counts are independent. Setting `query_threads` does not
+  implicitly change `optimize_threads`.
+
+Before this fix, the DB passed its configured count as a single constructor
+argument. Every valid positive count was converted to `true`, so the actual
+runtime behavior was hardware-concurrency threads with binding enabled. This
+meant configured values such as `optimize_threads=1` were ignored.
+
+After this fix, the configured count and binding are both forwarded
+explicitly, so the effective behavior matches the configuration. Binding
+remains enabled by default to preserve the previous effective behavior; users
+can explicitly disable it when scheduler-managed placement is preferable.
+
+## Python API
+
+`zvec.init()` exposes the four independent options:
+
+```python
+zvec.init(
+    query_threads=None,
+    query_thread_binding=None,
+    optimize_threads=None,
+    optimize_thread_binding=None,
+)
+```
+
+Behavior:
+
+| Input | Effective behavior |
+|---|---|
+| Count is `None` | Use the corresponding cgroup-derived DB default |
+| Count is a positive integer | Use exactly that many threads |
+| Binding is `None` | Use the DB default (`True`) |
+| Binding is `False` | Do not bind worker threads |
+| Binding is `True` | Bind worker threads where supported |
+
+Examples:
+
+```python
+# Both pools use their cgroup-derived counts with binding.
+zvec.init()
+
+# Optimize uses exactly one bound worker.
+zvec.init(optimize_threads=1)
+
+# Explicitly let the OS schedule both pools without affinity pinning.
+zvec.init(query_thread_binding=False, optimize_thread_binding=False)
+```
+
+## C API
+
+The C configuration API now includes independent setters and getters:
+
+```c
+zvec_config_data_set_query_thread_binding(config, binding);
+zvec_config_data_get_query_thread_binding(config);
+zvec_config_data_set_optimize_thread_binding(config, binding);
+zvec_config_data_get_optimize_thread_binding(config);
+```
+
+If these setters are not called, binding remains enabled.
+
+## Internal call-site migration
+
+Internal call sites were migrated according to their previous intended
+behavior:
+
+- Benchmark tools with no requested count still use hardware concurrency
+  without binding.
+- Recall tools with no requested count still use hardware concurrency with
+  binding.
+- Explicit benchmark and recall thread counts are now honored.
+- `Index::Merge` uses `write_concurrency` with binding, preserving its previous
+  effective affinity behavior.
+- DB query and optimize pools use their configured count and binding.
+- Existing call sites already passing both count and binding are unchanged.
+
+The recall resize path previously used the arguments in reverse order:
+
+```cpp
+ThreadPool(true, threads);
+```
+
+That constructed one bound worker. It now correctly uses:
+
+```cpp
+ThreadPool(threads, true);
+```
+
+## Performance validation
+
+The fix was validated with an end-to-end Python DB API benchmark using the
+SIFT dataset. The measured post-fix revision used disabled binding defaults.
+The current compatibility-preserving defaults keep binding enabled, so the
+post-fix placement measured below is reproduced by explicitly setting both
+binding options to `False`.
+
+| Parameter | Value |
+|---|---|
+| Dataset | SIFT, 1,000,000 FP32 vectors, 128 dimensions |
+| Index | Vamana |
+| Search list size | 500 |
+| Alpha | 1.5 |
+| Quantizer | Uniform INT8 |
+| Optimize threads | 1 per process |
+| Build flow | `create_and_open -> insert -> optimize` |
+
+Two codebases were compared on the same machine with separately built wheels
+and virtual environments:
+
+- Before the fix: commit `a90ec5b1d658d`, wheel `0.5.2.dev30`.
+- After the fix: the ThreadPool fix and migrated call sites, wheel
+  `0.5.2.dev32`.
+
+### Observed runtime behavior
+
+The test machine exposes 64 logical CPUs. The benchmark called:
+
+```python
+zvec.init(optimize_threads=1)
+```
+
+It did not override the query thread count or either binding option. In that
+measured revision the effective binding defaults were `False`; with the
+current defaults, use the following equivalent configuration for the post-fix
+run:
+
+```python
+zvec.init(
+    optimize_threads=1,
+    query_thread_binding=False,
+    optimize_thread_binding=False,
+)
+```
+
+| Pool / behavior | Before the fix | After the fix |
+|---|---|---|
+| Query pool | 64 workers, bound to CPUs 0-63 | 64 workers, unbound |
+| Optimize pool | 64 workers, bound to CPUs 0-63 | 1 worker, unbound |
+| Total pool workers | 128 | 65 |
+| Observed process threads | Approximately 214 | Approximately 150 |
+| Workers doing Vamana build work | Effectively about 1 per process | 1 per process |
+| CPU used by each builder in the four-process phase | Approximately 25% of one CPU before competing builders completed | Approximately 92-100% of one CPU |
+
+The total process thread count is higher than the two global pools because it
+also includes Python, storage-engine, I/O, and other background threads. Those
+additional threads were present in both versions.
+
+Before the fix, both positive configuration values were passed through the
+one-argument constructor:
+
+```cpp
+ThreadPool(configured_count);
+```
+
+Both `64` and `1` converted to the Boolean value `true`. Consequently, both
+pools created 64 workers and enabled binding. `optimize_threads=1` therefore
+did not create one optimize worker.
+
+The Vamana build remained effectively single-threaded in this test: despite
+the 64 optimize workers, only about one worker per process was doing sustained
+CPU work. The other optimize workers and the entire query pool were mostly
+idle. This explains why the single-process result did not become faster in the
+old version.
+
+The excessive idle workers add thread stacks, scheduler bookkeeping, and some
+context-switch overhead, but they are not sufficient to explain the observed
+four-times slowdown. The dominant problem was CPU affinity:
+
+1. Every process created the same 64-worker optimize pool.
+2. Worker `i` in every process was bound to CPU `i`.
+3. The effectively active worker selected by each identical pool tended to
+   have the same worker index.
+4. Active build work from multiple processes therefore competed on the same
+   CPU instead of being spread across the 64 available CPUs.
+
+This behavior matches the timing curve. The degree-24 build, which overlapped
+with all four processes for almost its entire lifetime, took 4.28 times its
+single-process time before the fix. As shorter jobs completed, the remaining
+processes received a larger share of the contended CPU, so the longest
+degree-64 job showed a smaller but still substantial 2.45-times slowdown.
+
+After the fix, each process created one unbound optimize worker. The operating
+system could schedule the four runnable workers on different CPUs, and live
+sampling showed each builder consuming close to one full CPU. The query pool
+still contributed 64 mostly idle workers because the benchmark did not set
+`query_threads`; this accounts for much of the approximately 150-thread process
+total, but did not prevent scaling because those threads were unbound and idle.
+
+### Single-process results
+
+| Degree | Before | After | Change |
+|---:|---:|---:|---:|
+| 24 | 790.2 s | 777.8 s | -1.6% |
+| 64 | 2865.3 s | 2847.0 s | -0.6% |
+
+Single-process performance was effectively unchanged. This indicates that the
+fix does not alter Vamana's underlying build cost or introduce a measurable
+single-process regression.
+
+### Four-process results
+
+Four independent processes simultaneously built indexes with degrees 24, 32,
+48, and 64. Every process requested `optimize_threads=1`.
+
+| Degree | Before | After | Speedup | Time reduction |
+|---:|---:|---:|---:|---:|
+| 24 | 3378.6 s | 841.6 s | 4.01x | 75.1% |
+| 32 | 4282.3 s | 1137.5 s | 3.76x | 73.4% |
+| 48 | 5955.2 s | 1946.4 s | 3.06x | 67.3% |
+| 64 | 7009.0 s | 2975.1 s | 2.36x | 57.6% |
+
+The four-process group wall-clock time was determined by the degree-64 build:
+
+| Metric | Before | After | Improvement |
+|---|---:|---:|---:|
+| Group wall time | 7009.2 s (116m 49s) | 2975.4 s (49m 35s) | 57.5% lower |
+| Equivalent speedup | — | — | 2.36x |
+
+### Parallel overhead relative to a single process
+
+| Degree | Before fix | After fix |
+|---:|---:|---:|
+| 24 | +327.6% | +8.2% |
+| 64 | +144.6% | +4.5% |
+
+Before the fix, the degree-24 build took 4.28 times as long when sharing the
+machine with three other builders. After the fix it took only 1.08 times as
+long. The degree-64 ratio similarly improved from 2.45 times to 1.05 times.
+
+All indexes produced before and after the fix reached an index completeness of
+1.0. The results confirm that the change specifically removes multi-process
+thread contention while preserving single-process performance and index
+correctness.
+
+## Compatibility summary
+
+Behavior is unchanged for:
+
+- `ThreadPool()`;
+- `ThreadPool(n, false)`;
+- `ThreadPool(n, true)`;
+- `Index::Merge` CPU binding;
+- DB query and optimize pool binding defaults;
+- internal call sites whose old behavior was already explicit and correct.
+
+Behavior intentionally changes for:
+
+- one-argument thread counts that were previously interpreted as booleans;
+- DB thread-count configuration that was previously ignored;
+- recall resize paths affected by reversed arguments.
+
+DB binding remains enabled by default, matching the previous effective
+runtime behavior. Adding binding fields to the public C++ `ConfigData` struct
+changes its layout, so consumers linking across a C++ ABI boundary must rebuild
+against the updated headers.

python/tests/detail/test_db_config.py

@@ -153,6 +153,21 @@ class TestDbConfigThreadValidation:
     def test_query_threads(self):
         zvec.init(query_threads=1)
 
+    @run_in_subprocess
+    def test_query_thread_binding(self):
+        zvec.init(query_thread_binding=True)
+
+    @run_in_subprocess
+    def test_query_thread_binding_disabled(self):
+        zvec.init(query_thread_binding=False)
+
+    @run_in_subprocess
+    def test_query_thread_binding_invalid(self):
+        with pytest.raises(TypeError):
+            zvec.init(query_thread_binding=1)
+        with pytest.raises(TypeError):
+            zvec.init(query_thread_binding="true")
+
     @run_in_subprocess
     def test_query_threads_invalid(self):
         # query_threads must >= 0 and must be int and if None, set default value
@@ -171,6 +186,21 @@ def test_query_threads_invalid(self):
     def test_optimize_threads(self):
         zvec.init(optimize_threads=1)
 
+    @run_in_subprocess
+    def test_optimize_thread_binding(self):
+        zvec.init(optimize_thread_binding=True)
+
+    @run_in_subprocess
+    def test_optimize_thread_binding_disabled(self):
+        zvec.init(optimize_thread_binding=False)
+
+    @run_in_subprocess
+    def test_optimize_thread_binding_invalid(self):
+        with pytest.raises(TypeError):
+            zvec.init(optimize_thread_binding=1)
+        with pytest.raises(TypeError):
+            zvec.init(optimize_thread_binding="true")
+
     @run_in_subprocess
     def test_optimize_threads_invalid(self):
         # optimize_threads must >= 0 and must be int and if None, set default value