Cap LT/LOAD/BRANCH/MEMW_R max_rows at 2^20#499
Conversation
Tables with effective width < 42 were sized at 2^21, producing single large chunks. Capping at 2^20 produces 2x more chunks of the standard size, improving parallel throughput and keeping peak memory per chunk uniform across all tables.
|
/bench |
Codex Code ReviewNo actionable issues found in this PR diff. Residual risk/testing gap:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: Cap LT/LOAD/BRANCH/MEMW_R max_rows at 2^20Overall: Clean, minimal change. The rationale is sound — capping these four narrow-width tables at 2^20 instead of the formula-derived 2^21 doubles chunk count for better parallel throughput and keeps per-chunk peak memory uniform. One issue[Low — Documentation] The newly added paragraph ends with:
This is incorrect. These constants are chunk sizes, not absolute row limits. In Everything else looks good
|
Benchmark — fib_iterative_8M (median of 3)Table parallelism: 32 (auto = cores / 3)
Commit: d36b05b · Baseline: built from main · Runner: self-hosted bench |
Codex Code ReviewNo issues found in the PR diff. I reviewed only the changes in
The change is a clear, intentional behavioral cap ( |
Review: Cap LT/LOAD/BRANCH/MEMW_R max_rows at 2^20The change is clean and well-motivated. The docs are clearly updated and the rationale is sound: narrow tables produced 2^21-row single-threaded chunks; halving to 2^20 produces standard-sized chunks that parallelize properly. Potential bug / Breaking change The new cap cuts the per-table row capacity in half. Programs with between 2^20 and 2^21 rows in any of LT, LOAD, BRANCH, or MEMW_R now fail with a table-overflow error where they previously succeeded. The doc comment acknowledges this ( A CPU table capped at 2^19 means at most 524 288 CPU steps, so hitting 2^20 LOAD rows requires on average >2 LOAD ops per CPU step (e.g. unaligned loads that split into multiple entries). Whether that's reachable in practice is worth explicitly checking against your benchmark suite before merging. No tests for new threshold There are no tests that verify overflow fires at 2^20 rather than 2^21 for these tables. Given that this is a deliberate capacity reduction, a regression test covering the new boundary would be valuable. Minor doc nit The PR description says "Tables with effective width < 42" but LT has effective width exactly 42 (not strictly less than). Should be "≤ 42" or just list the four tables explicitly. |
* perf(tables): cap LT/LOAD/BRANCH/MEMW_R max_rows at 2^20 Tables with effective width < 42 were sized at 2^21, producing single large chunks. Capping at 2^20 produces 2x more chunks of the standard size, improving parallel throughput and keeping peak memory per chunk uniform across all tables. * Drop incorrect overflow claim from max_rows doc * Drop (capped) annotations from max_rows doc --------- Co-authored-by: diegokingston <dkingston@fi.uba.ar>
3 participants
Tables with effective width < 42 were sized at 2^21, producing single large chunks. Capping at 2^20 produces 2x more chunks of the standard size, improving parallel throughput and keeping peak memory per chunk uniform across all tables.