Skip to content

perf: merge each unique subgraph object once, not once per occurrence - #45

Closed
AmitMY wants to merge 1 commit into
mainfrom
perf/merge-unique-subgraphs
Closed

AmitMY wants to merge 1 commit into
mainfrom
perf/merge-unique-subgraphs

Conversation

@AmitMY

@AmitMY AmitMY commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What

Gigatoken-inspired (marcelroed/gigatoken): its BPE trainer never iterates word occurrences — everything is keyed on unique words with counts. Our equivalent: duplicated words already share one subgraph object (the _build_graphs cache), but UnconnectedGraphs.merge re-ran the merge once per occurrence. Worse, on the first merge that changed a word, its occurrences diverged into equal-but-distinct copies — each then re-memoizing get_merges and re-scanning merges separately for the rest of training (measured: 16 unique words ballooning to ~290 distinct objects mid-training).

Now merge computes each distinct object's result once and reuses it for every occurrence, so duplicates stay shared (one object, one memo) for the whole run.

Benchmarks (Python 3.14, best of 3, byte-identical digests)

Zipf-distributed corpus (20k words, 2k-word vocabulary, seeded):

Case main PR Δ time Δ peak memory
BPE (300 merges) 1.92s / 35.7MB 1.61s / 31.2MB −16% −13%
BNE n=4 (300 merges) 2.98s / 37.8MB 2.33s / 32.2MB −22% −15%
Boundless (100 merges) 1.08s / 37.0MB 1.06s / 37.0MB neutral neutral

Honest tradeoff: an adversarial corpus where every word is unique pays ~+12% (0.39s → 0.46s) for the per-step unique-object dict. Real text is Zipf-distributed, so duplication is the norm; the win grows with corpus size / dup ratio.

Verification

  • 138 tests pass, ruff clean
  • Merge digests byte-identical to main on all benchmark cases

🤖 Generated with Claude Code

Duplicated words share one subgraph object via the build cache, but
UnconnectedGraphs.merge re-ran the merge per occurrence, so on the first
merge that changed a word its occurrences diverged into equal-but-distinct
copies, each re-memoizing get_merges separately. Merging per unique object
keeps duplicates shared for the whole training run.

Zipf corpus (20k words, 2k vocab), byte-identical output:
- BPE: 1.92s -> 1.61s (-16%), peak 35.7MB -> 31.2MB (-13%)
- BNE: 2.98s -> 2.33s (-22%), peak 37.8MB -> 32.2MB (-15%)
- Boundless: neutral

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@AmitMY

AmitMY commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the trainer-level version in #42 (per review discussion): deduping once in the trainer and training on unique subgraphs with weights is both simpler and faster than deduping inside UnconnectedGraphs.merge every step — BPE 0.074s vs 1.61s here, and it removes this PR's +12% overhead on corpora with no duplicate words. Keeping graph.py untouched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant