|
| 1 | +# OMC Pain Points — Real-World Stress Test (MovieLens 10k) |
| 2 | + |
| 3 | +Live-captured findings from building a real movie recommendation |
| 4 | +engine over MovieLens latest-small (10k rating subset of 100k full). |
| 5 | +Each entry: severity, evidence, root cause, suggested fix. |
| 6 | + |
| 7 | +Severity scale: |
| 8 | +* **CRIT** — Wrong-output bug. Silent. Will bite users. |
| 9 | +* **HIGH** — Performance cliff that prevents real-world use. |
| 10 | +* **MED** — Ergonomic friction that adds 2× to development time. |
| 11 | +* **LOW** — Polish; cosmetic or minor. |
| 12 | + |
| 13 | +Sorted approximately by impact. |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## CRIT-1: Float truncation in arr_get / dict_get arithmetic ✅ FIXED |
| 18 | + |
| 19 | +**Symptom:** `arr_get(cur, 1) + rating` where `rating` is a float |
| 20 | +silently truncates the result to int. Aggregating ratings 4.5 + 3.5 |
| 21 | ++ 5.0 returned 12 (or 13 depending on engine), not 13.0. |
| 22 | + |
| 23 | +**Root cause:** The compiler's static return-type table at |
| 24 | +`compiler.rs:140-141` claimed `arr_get / dict_get / arr_min / arr_max |
| 25 | +/ arr_sum` always return int. They're polymorphic over element type. |
| 26 | +The lie made the compiler emit `Op::AddInt` (typed fast-path), which |
| 27 | +calls `.to_int()` on both operands → silent float→int truncation. |
| 28 | + |
| 29 | +**Engine divergence:** Tree-walk and VM produced *different* wrong |
| 30 | +answers (16 vs 12, 523 vs 782 hits) because tree-walk's eval_expr |
| 31 | +uses runtime types and VM's bytecode uses compile-time types. Made |
| 32 | +it look like a "VM bug" when both engines were affected. |
| 33 | + |
| 34 | +**Fix:** Removed polymorphic builtins from the int-return table. |
| 35 | +Commit `d792672`. |
| 36 | + |
| 37 | +**Lesson:** Static type inference for collection accessors is unsound |
| 38 | +unless the type system tracks element types — which OMC's doesn't. |
| 39 | +Default to "no inference" for any builtin whose return depends on |
| 40 | +input data; only inline fast-paths for builtins with truly fixed |
| 41 | +return types (arr_len, str_len, fibonacci, etc). |
| 42 | + |
| 43 | +--- |
| 44 | + |
| 45 | +## HIGH-1: Value::Dict / Value::Array clone on every read+write |
| 46 | + |
| 47 | +**Symptom:** Aggregating 10k records into a dict that grows to 3218 |
| 48 | +entries takes 16 seconds. 100k records: hung — never completed in |
| 49 | +several minutes. Same pattern affects `arr_push` on growing arrays |
| 50 | +(0.4s for 10k integer pushes — should be ~10ms). |
| 51 | + |
| 52 | +**Evidence (10k):** |
| 53 | +``` |
| 54 | +load_csv: 9899 ms |
| 55 | +aggregate: 16018 ms ← THIS |
| 56 | +agg_to_rows: 2125 ms |
| 57 | +build_hidx: 4883 ms ← AND THIS |
| 58 | +linear scan: 1185 ms |
| 59 | +``` |
| 60 | + |
| 61 | +**Root cause:** `Value::Dict(BTreeMap<...>)` and `Value::Array(...)` |
| 62 | +both `derive(Clone)`. Every Op::DictSetNamed / arr_push / dict_get |
| 63 | +invokes vm_get_var → vm_assign_var, each of which clones the entire |
| 64 | +backing collection. For a dict growing to N entries, each iteration |
| 65 | +of the loop costs O(N) — the whole thing is O(N²). |
| 66 | + |
| 67 | +For 10k records building a 3k-entry dict: |
| 68 | +- ~20k clones during the loop |
| 69 | +- Avg clone size ~1.5k entries |
| 70 | +- ~30M element copies total → ~16 seconds |
| 71 | + |
| 72 | +**Suggested fix (architectural):** Wrap collections in |
| 73 | +`Rc<RefCell<...>>`. clone becomes O(1) (Rc bump). Mutation through |
| 74 | +vm_assign_var becomes a borrow_mut() into the shared backing. |
| 75 | +Semantic implication: dicts/arrays become *shared by reference* like |
| 76 | +closure environments, not pass-by-value. This matches Python's |
| 77 | +reference semantics for dict/list and unblocks any algorithm that |
| 78 | +builds collections in a loop. |
| 79 | + |
| 80 | +**Cheaper interim fix:** Add a builtin-fused `dict_update(d, k, |
| 81 | +fn)` that does in-place modify (one clone, not two), and a similar |
| 82 | +`arr_extend` that does bulk-push. Speeds the common patterns ~2× but |
| 83 | +doesn't escape the O(N²). |
| 84 | + |
| 85 | +**Lesson:** "Pass-by-value" was a defensible early choice (matches |
| 86 | +arrays' existing semantics). At 10k+ scale it stops working. The |
| 87 | +architecture needs to choose: pass-by-value (O(N²) collections) or |
| 88 | +pass-by-reference (sharing, mutation surprises). Document the |
| 89 | +tradeoff and pick. |
| 90 | + |
| 91 | +--- |
| 92 | + |
| 93 | +## HIGH-2: str_split per-line cost dominates CSV parsing |
| 94 | + |
| 95 | +**Symptom:** `load_csv` is 10s for 10k lines; 6.4s of that is |
| 96 | +calling `str_split(line, ",")` 10k times. |
| 97 | + |
| 98 | +**Root cause:** Each `str_split` call goes through vm_call_builtin |
| 99 | +(or vm_fast_dispatch) and allocates a fresh `Vec<Value::String>`. |
| 100 | +40k Value::String allocations + 40k Value pushes through the VM |
| 101 | +stack. The actual `s.split(",")` is fast; the wrapping overhead |
| 102 | +isn't. |
| 103 | + |
| 104 | +**Suggested fix:** Add a `csv_parse(text)` builtin that does the |
| 105 | +entire parse in one call — returns `Array<Array<String>>` directly. |
| 106 | +Eliminates 10k VM round-trips. Should bring 10k-line load under 100ms. |
| 107 | + |
| 108 | +**Generalization:** Any "I'm doing the same VM-mediated thing 10k |
| 109 | +times" pattern needs a vectorized builtin. Same applies to mapping |
| 110 | +to_int across an array (could be `arr_to_int(strings)`). |
| 111 | + |
| 112 | +--- |
| 113 | + |
| 114 | +## HIGH-3: VM is *slower* than tree-walk on dict-heavy code |
| 115 | + |
| 116 | +**Evidence:** |
| 117 | + |
| 118 | +| Workload | tree-walk | VM | ratio | |
| 119 | +|---|---|---|---| |
| 120 | +| 10k aggregate (dict-heavy) | 16s | 18-20s | 1.13× *slower* | |
| 121 | +| 10k build_hidx | 4.9s | 6.7s | 1.37× *slower* | |
| 122 | +| HOF arr_map (Phase 4 bench) | 131ms | 59ms | **2.22× faster** | |
| 123 | + |
| 124 | +**Root cause:** The Op::DictSetNamed path goes vm_get_var → mutate |
| 125 | +→ vm_assign_var. Both steps clone the dict. The tree-walk path |
| 126 | +does the same number of clones, but its eval_expr tail-calls are |
| 127 | +slightly cheaper than the VM's stack-machine bookkeeping when both |
| 128 | +are bottlenecked on identical Rust-side work. |
| 129 | + |
| 130 | +**Implication:** The Phase 4 win ("VM ≥ tree-walk on every |
| 131 | +benchmark") is true for the Phase 4 benchmarks but doesn't |
| 132 | +generalize. Anything that sits in the same dict-clone hot loop sees |
| 133 | +no benefit from the bytecode VM — and pays its dispatch overhead. |
| 134 | + |
| 135 | +**Fix:** Same as HIGH-1. Once dict mutation is O(1) (Rc-shared), |
| 136 | +the VM's hot dispatch should win again because builtin calls amortize. |
| 137 | + |
| 138 | +--- |
| 139 | + |
| 140 | +## MED-1: arr_push in a hot loop is silently O(N²) |
| 141 | + |
| 142 | +Already covered under HIGH-1, but worth calling out: |
| 143 | + |
| 144 | +**Symptom:** "Build an array of N records" takes O(N²) time. |
| 145 | + |
| 146 | +**User-facing impact:** Anything that follows the standard pattern |
| 147 | + |
| 148 | +```omc |
| 149 | +h out = []; |
| 150 | +while ... { |
| 151 | + arr_push(out, item); |
| 152 | +} |
| 153 | +return out; |
| 154 | +``` |
| 155 | + |
| 156 | +stops working past ~5000 iterations. There's no syntactic |
| 157 | +indication that this is the wrong pattern. |
| 158 | + |
| 159 | +**Suggested fix:** Either fix HIGH-1 architecturally, OR teach |
| 160 | +users an alternative pattern (`arr_new(N, default)` + index assign, |
| 161 | +or a builder type). Either way, document the cliff. |
| 162 | + |
| 163 | +--- |
| 164 | + |
| 165 | +## MED-2: harmonic_index hit count is misleading vs linear scan |
| 166 | + |
| 167 | +**Evidence (10k):** |
| 168 | +``` |
| 169 | +linear (R≈4 ±0.1): 523 hits |
| 170 | +harmonic (R≈4 by attractor): 1825 hits |
| 171 | +``` |
| 172 | + |
| 173 | +The harmonic engine returns 3.5× as many hits as the linear scan |
| 174 | +because the attractor bucket for `target * 100 = 400` folds to 377 |
| 175 | +and includes everything in roughly [277.5, 472.5] — a much wider |
| 176 | +range than ±0.1. |
| 177 | + |
| 178 | +**Not a bug** — it's the correct semantics of harmonic |
| 179 | +neighborhood lookup. But it makes "compare engines" benchmarks |
| 180 | +misleading. |
| 181 | + |
| 182 | +**Suggested fix:** Document the intent more clearly. The harmonic |
| 183 | +engine is a *coarse* index (sub-linear lookup → coarse bucket); the |
| 184 | +linear scan is *fine* (exact distance → narrow bucket). For a |
| 185 | +recommendation system this is great (more diversity for free), but |
| 186 | +for an "exact lookup" it's wrong. |
| 187 | + |
| 188 | +--- |
| 189 | + |
| 190 | +## MED-3: OMC_HEAL would silently rewrite domain values |
| 191 | + |
| 192 | +**Evidence:** |
| 193 | +``` |
| 194 | +$ OMC_HEAL=1 ./omc examples/recommend/recommend.omc |
| 195 | +--- OMC_HEAL: 1 diagnostic(s) across 1 iteration(s) (converged) --- |
| 196 | + harmonic: 4 not Fibonacci → 3 (|Δ|=1) |
| 197 | +--- end OMC_HEAL --- |
| 198 | +``` |
| 199 | + |
| 200 | +The heal pass saw the literal `4` (used as `target = 4.0` in our |
| 201 | +"recommend movies near rating 4") and helpfully rewrote it to `3` |
| 202 | +(the nearest Fibonacci) — which would have meant the user query |
| 203 | +"movies rated 4 stars" became "movies rated 3 stars." Coincidentally |
| 204 | +the run completed identically, suggesting the heal didn't actually |
| 205 | +trigger on the rating-4.0 expression (possibly due to it being a |
| 206 | +float literal, not int), but the diagnostic firing on a 4 *somewhere* |
| 207 | +in the file is concerning. |
| 208 | + |
| 209 | +**Root cause:** The harmonic-rewrite rule fires on any int literal |
| 210 | +within edit-distance 3 of a Fibonacci attractor, with no awareness |
| 211 | +of *what the value means*. |
| 212 | + |
| 213 | +**Suggested fix:** Heal pass should respect a `@no_heal` decoration |
| 214 | +on functions/expressions, OR only fire when the literal appears in a |
| 215 | +position where an attractor would make sense (e.g., array indexing, |
| 216 | +not comparison RHS). For now, treat OMC_HEAL as opt-in per-file. |
| 217 | + |
| 218 | +--- |
| 219 | + |
| 220 | +## MED-4: No way to import a single fn from another file |
| 221 | + |
| 222 | +**Symptom:** I copy-pasted four `hidx_*` functions from |
| 223 | +`examples/harmonic_collections.omc` into `examples/recommend/recommend.omc` |
| 224 | +because OMC doesn't support `from "x" import y`. The full |
| 225 | +`import "x" as alias` form imports *every* function and aliases the |
| 226 | +whole module — too heavyweight when you want one helper. |
| 227 | + |
| 228 | +**Suggested fix:** Either add `from "path" import name1, name2;` or |
| 229 | +make `import "path" as alias` accept the alias as `*` or empty |
| 230 | +("merge selected names into namespace"). |
| 231 | + |
| 232 | +--- |
| 233 | + |
| 234 | +## LOW-1: Float display drops trailing `.0` for whole numbers |
| 235 | + |
| 236 | +**Evidence:** `println(3.0)` prints `3`. Same for `count=1 avg=4` |
| 237 | +which suggested an int when avg was actually a float. |
| 238 | + |
| 239 | +**Root cause:** Rust's `format!("{}", 3.0_f64)` produces `"3"`. We |
| 240 | +inherit this in `Value::to_display_string`. |
| 241 | + |
| 242 | +**Suggested fix:** `Value::HFloat(f)` display should always show a |
| 243 | +decimal point. e.g., `format!("{:?}", f)` produces `"3.0"`. Trade: |
| 244 | +all float output is slightly noisier; benefit: int-vs-float ambiguity |
| 245 | +in user output disappears. |
| 246 | + |
| 247 | +--- |
| 248 | + |
| 249 | +## LOW-2: Engine-divergence error reports are useless |
| 250 | + |
| 251 | +When the float bug was active, both engines silently produced wrong |
| 252 | +answers. The user has no way to know "this output is wrong" without |
| 253 | +running both engines and diffing. We have a regression sweep in dev, |
| 254 | +but a real user wouldn't. |
| 255 | + |
| 256 | +**Suggested fix:** Add `--audit` mode that runs both engines on the |
| 257 | +same input and flags ANY divergence in output. Lightweight CI tool. |
| 258 | + |
| 259 | +--- |
| 260 | + |
| 261 | +## LOW-3: Performance reporting in user code is verbose |
| 262 | + |
| 263 | +Compare: `now_ms()` paired with subtraction is the only timing tool. |
| 264 | +Every benchmark stanza in the recommend.omc is 4 lines of boilerplate |
| 265 | +for one timed step. |
| 266 | + |
| 267 | +**Suggested fix:** A `time_block(label, fn)` builtin that wraps a |
| 268 | +closure, runs it, prints `label: Xms`, returns the result. Saves 3 |
| 269 | +lines per timed step. |
| 270 | + |
| 271 | +--- |
| 272 | + |
| 273 | +# Prioritized fix list |
| 274 | + |
| 275 | +1. **HIGH-1** (Rc-shared collections) — unblocks 10k+ workloads |
| 276 | +2. **HIGH-2** (`csv_parse` builtin) — unblocks loading large data |
| 277 | +3. **CRIT-1** (float truncation) ✅ FIXED |
| 278 | +4. **MED-3** (OMC_HEAL respects literal context) — silent semantic bugs |
| 279 | +5. **MED-4** (selective imports) — every multi-file demo wants this |
| 280 | +6. **HIGH-3** (VM dict perf) — automatic from HIGH-1 |
| 281 | +7. **LOW-1** through **LOW-3** — polish |
| 282 | + |
| 283 | +# What surprised me |
| 284 | + |
| 285 | +* The float bug had been latent forever and would have shipped to a |
| 286 | + real user the first time they aggregated floats through arr_get. |
| 287 | + Found in 5 minutes of running real code. **Real datasets are the |
| 288 | + test harness; toy demos miss everything.** |
| 289 | + |
| 290 | +* Tree-walk and VM produce *different wrong answers* under the same |
| 291 | + bug. That's worse than both being wrong the same way — there's no |
| 292 | + ground truth to diff against. |
| 293 | + |
| 294 | +* The harmonic_index *worked* on real data — buckets concentrated on |
| 295 | + the natural rating attractors (3.5, 4.0, 4.5). But the n² collection |
| 296 | + cost meant we couldn't even build it past 10k records. The |
| 297 | + language is the bottleneck, not the algorithm. |
| 298 | + |
| 299 | +* `OMC_HEAL` actively makes things wrong on real data. It's a |
| 300 | + research-fun feature on isolated demos but unsafe-by-default for |
| 301 | + real programs. **Opt-in per-file decoration is the right move.** |
0 commit comments