Repository navigation
Conversation
…d indexer Only `python -m dynamo.frontend` can preload jemalloc today, through DYN_FRONTEND_JEMALLOC. The standalone router and the standalone KV indexer have no opt-in and need a hand-written LD_PRELOAD with the library path. Move the frontend's preload helper into the stdlib-only module dynamo._jemalloc in the runtime wheel, so every entry point can call it before importing the Rust extension, including dynamo.indexer, which ships in that wheel. The helper reads the new DYN_JEMALLOC. An entry point can also pass a component-specific alias; only the frontend passes DYN_FRONTEND_JEMALLOC, and it preloads when either variable is enabled. Truthy parsing, already-preloaded detection, the missing-library warning, and the single re-exec are unchanged. Warnings now name the variable that enabled the preload. Call the helper before the runtime import in dynamo.frontend, dynamo.router, and dynamo.indexer. The router's module body moves verbatim from __main__.py to main.py, as in the frontend, so `python -m dynamo.router` keeps the same CLI. jemalloc stays opt-in. Signed-off-by: PeaBrane <yanrpei@gmail.com>
github-actions
Bot
added
feat
documentation
Contributor
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review. WalkthroughThe change adds a shared jemalloc preload helper and wires it into the indexer, standalone router, and frontend entry points. It moves the standalone router service implementation from ChangesPython entry points and standalone router
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The documented entry points retain their expected behavior, and no actionable issue remains before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 27.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 9 files. (3 skipped: 3 unsupported.) Full details: Description checkExplanation The description provides detailed scope, implementation notes, behavior, documentation changes, and validation results. However, it omits the required Related Issues reference and does not include the template sections for Overview, Details, or reviewer starting points. Resolution Add the required Related Issues section with a valid issue reference, such as
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
This was referenced Oct 8, 2026
The already-preloaded and restart paths do not depend on which variable enabled the helper, so run them under DYN_JEMALLOC=1 only. Alias and selection coverage stays in test_alias_ignored_unless_passed and the ENABLED matrix in test_missing_library and test_exec_failure. Signed-off-by: PeaBrane <yanrpei@gmail.com>
Contributor
✅ Dynamo PR CI passed — run 37733043913 (attempt 1) on
|
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: The exec-failure test still runs the full ENABLED matrix against all three LD_PRELOAD states. The enabled-source warning contract is already retained by the ENABLED cases, while LD_PRELOAD preservation only needs absent, empty, and nonempty states under one enabled source, so the cross product remains non-discriminating.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status (2026-10-09). John Thomson is taking these over. This PR is independent of the indexer stack and ready to merge. If #15884 also merges, whichever lands second should collapse the
DYN_FRONTEND_JEMALLOCdocs entry into an alias ofDYN_JEMALLOC; the two entries currently repeat each other.Summary
Today only the frontend can opt in to jemalloc:
python -m dynamo.frontendreadsDYN_FRONTEND_JEMALLOC, addslibjemalloctoLD_PRELOAD, and re-execs itself once. No other component reads aDYN_FRONTEND_*variable. The standalone router (python -m dynamo.router, including--serve-indexer) and the standalone KV indexer (python -m dynamo.indexer) have no opt-in, so operators have to setLD_PRELOADto an architecture-specific library path by hand.This PR adds
DYN_JEMALLOC, a generic opt-in shared by all three entry points. It follows theDYN_<FEATURE>naming used for settings that apply across components.dynamo._jemalloc, a module that uses only the standard library.dynamo.indexerships there and cannot importai-dynamo.dynamo.common.utilswas not an option: importing it loads the Rust extension, and the preload must happen before that.dynamo._jemallocdoes not load the extension.1,trueoryes, case-insensitive;LD_PRELOADalready names alibjemalloc;execvewith jemalloc prepended toLD_PRELOAD.DYN_JEMALLOC, and an entry point can also pass its own alias.DYN_FRONTEND_JEMALLOC, so existing frontend deployments keep working. The frontend preloads when either variable is enabled.dynamo.frontend,dynamo.routeranddynamo.indexereach call the helper before importing the runtime.__main__.pytomain.py, matching the frontend's layout.python -m dynamo.routerkeeps the same CLI.maininstead of__main__.DYN_JEMALLOC, its use in the router and indexer, and the frontend-only alias.Validation
pytest lib/bindings/python/tests/test_jemalloc_preload.py, covering the shared helper::or space separators;execvereceivesLD_PRELOADwith jemalloc prepended and the original argv, after stdout and stderr are flushed;LD_PRELOADunchanged;dynamo.indexer.mainis imported.pytest components/src/dynamo/frontend/tests/test_jemalloc.py components/src/dynamo/router/tests/test_router_jemalloc.py components/src/dynamo/router/tests/test_standalone_router.py:__main__without running it does not restart;DYN_JEMALLOCre-execs before the runtime import, andDYN_FRONTEND_JEMALLOCis ignored.os.execvestubbed shows thatdynamo.frontend,dynamo.routeranddynamo.indexerload onlydynamo,dynamo._jemallocand their own package before the re-exec.dynamo._coreis not loaded.kv-indexerfeature) andlibjemallocon the library path.MALLOC_CONF=stats_print:truewas used to confirm that jemalloc loaded:DYN_JEMALLOC=1 python -m dynamo.indexer --helploads jemalloc and re-execs exactly once.DYN_FRONTEND_JEMALLOC=1, does not load jemalloc.DYN_JEMALLOC=1 python -m dynamo.router --helploads jemalloc, and its help output is identical to the previous module's.python -m dynamo.frontend --helploads jemalloc with either variable.pre-commit run --files <changed files>, mypy on the touched modules, anddocs/fern/scripts/docs_lint.py.Summary by CodeRabbit