perf(binding): reduce module graph connection traversal overhead#13226
perf(binding): reduce module graph connection traversal overhead#13226
Conversation
github-actions
bot
added
release: performance
There was a problem hiding this comment.
Pull request overview
This PR optimizes the N-API JsModuleGraph traversal APIs by reducing per-connection hash lookups when collecting outgoing/incoming connections, targeting a known performance hotspot in module graph traversal from JS.
Changes:
- Update
get_outgoing_connections/get_incoming_connectionsto iterate dependency IDs directly fromModuleGraphModuleedge sets. - Preserve the reusable
connection_vec_bufferapproach to avoid repeatedVecreallocations during N-API array construction.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| let mut arr = env.create_array(vec.len() as u32)?; |
There was a problem hiding this comment.
Same buffer-reuse concern here: if env.create_array(...) errors, the function returns before vec.drain(..) runs, leaving connection_vec_buffer populated and potentially corrupting subsequent results. Consider vec.clear() before filling and clearing on the create_array error path.
| vec.reserve(module_graph_module.incoming_connections().len()); | ||
| for dependency_id in module_graph_module.incoming_connections() { |
There was a problem hiding this comment.
module_graph_module.incoming_connections() is called twice (for len() and iteration). Consider storing it in a local (let incoming = ...) and using it for both to reduce repeated calls/borrows and improve readability.
| vec.reserve(module_graph_module.incoming_connections().len()); | |
| for dependency_id in module_graph_module.incoming_connections() { | |
| let incoming = module_graph_module.incoming_connections(); | |
| vec.reserve(incoming.len()); | |
| for dependency_id in incoming { |
| } | ||
| } | ||
| let mut arr = env.create_array(vec.len() as u32)?; |
There was a problem hiding this comment.
connection_vec_buffer is reused across calls, but this function can return early on env.create_array(...) error, leaving the buffer populated (since drain(..) hasn’t run yet). That can cause subsequent calls to include stale/duplicated connections. Clear the buffer before filling and also clear it on the create_array error path.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8695df1345
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for dependency_id in module_graph_module.outgoing_connections() { | ||
| vec.push(ModuleGraphConnectionWrapper::new( | ||
| *dependency_id, |
There was a problem hiding this comment.
get_outgoing_connections (and the matching get_incoming_connections change) now wraps every ID from ModuleGraphModule edge sets directly, but those sets can contain dependency IDs whose connection has already been removed (the core code explicitly handles this with filter_map, e.g. stats/mod.rs notes "the connection is removed"). Previously this binding called module_graph.get_*_connections(), which already filtered missing connections; after this change, JS can receive ModuleGraphConnection objects backed by missing entries, and accessing fields like .module/.resolvedModule will throw from module_graph_connection.rs instead of returning a valid connection list.
Useful? React with 👍 / 👎.
Rsdoctor Bundle Diff AnalysisFound 5 projects in monorepo, 5 projects with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 react-10kPath:
📁 react-1kPath:
📁 romePath:
📁 react-5kPath:
📁 ui-componentsPath:
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
🎉 Size decreased by 256bytes from 48.96MB to 48.96MB (⬇️0.00%) |
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
📝 Benchmark detail: Open
|
Summary
JsModuleGraph.get_outgoing_connectionsandget_incoming_connectionsto iterate dependency ids directly fromModuleGraphModuleedge sets instead of materializing iterators that repeatedly probeconnection_by_dependency_idconnection_vec_bufferpath so N-API array construction still avoids per-call Vec reallocations while cutting extra hash lookups and transient workPerformance Verification
pnpm run build:binding:devtests/bench/fixtures/ts-react, 30,000 rounds of:moduleGraph.getOutgoingConnections(module)moduleGraph.getOutgoingConnectionsInOrder(module)moduleGraph.getIncomingConnections(module)2647.09 ms2412.70 ms