-
Notifications
You must be signed in to change notification settings - Fork 5
🤖 Move tokenization to worker thread pool #266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
edd93b4
to
785d7b4
Compare
💡 Codex Reviewcmux/src/utils/main/tokenizer.ts Lines 198 to 230 in 8d10304
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
Thanks for the review! You're absolutely correct - the worker pool infrastructure was extracted from PR #247, but the IPC integration that actually calls it was left out (and is commented out in the original PR due to E2E test issues). This PR contains the worker pool infrastructure as preparatory code, but it's not yet wired up. I'll either:
Let me check with the user on which direction to take. |
✅ Fixed! I've added the full IPC integration:
The worker pool is now fully functional and will be invoked via IPC. Dynamic imports ensure it's only loaded when actually used. |
@codex review |
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
Extracted tokenization into a worker thread pool to prevent blocking the main process. This change improves responsiveness during token counting operations. Changes: - Added TokenizerWorkerPool service to manage worker lifecycle - Created tokenizerWorker for off-thread token counting - Updated tokenizer.ts to use worker pool instead of direct encoding - Updated tsconfig.main.json to include workers directory for compilation - Added tests for tokenizer caching behavior _Generated with `cmux`_
Added IPC integration to make the worker pool functional: - Added TOKENS_COUNT_BULK IPC channel constant - Updated IPCApi type with tokens.countBulk method - Added tokens API to preload.ts - Registered token handlers in ipcMain.ts - Added worker pool cleanup on app quit The worker pool is now fully wired up and will be called via IPC. Dynamic imports ensure the worker is only loaded when actually used.
The worker pool was causing Jest to hang because the worker thread prevented Node from exiting. Using unref() allows Node to exit even if the worker is still running, which is safe since the worker is lazily created and terminated on app quit. This fixes the integration test failures where tests would timeout waiting for the process to exit.
6c12807
to
eac57e9
Compare
✅ All tests passing! Integration tests now pass after adding The Codex comment was addressed in commit e0d326b where I added the full IPC integration. |
Extracted tokenization into a worker thread pool to prevent blocking the main process during token counting operations.
Changes
Benefits
Testing
bun test src/utils/main/tokenizer.test.ts
Generated with
cmux
Extracted from #247