Repository navigation
Bound how deep inlining nests - #8801
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ 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 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8801 +/- ##
=======================================
Coverage 80.40% 80.40%
=======================================
Files 447 447
Lines 60984 60990 +6
=======================================
+ Hits 49036 49042 +6
Misses 11948 11948
🚀 New features to boost your workflow:
|
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
1557d40 to
9912b5b
Compare
|
@cknitt What's your view on the trade-off between smaller generated code and runtime performance with the depth limit? Do you expect the extra calls to have a noticeable runtime cost? |
9912b5b to
cf56e8a
Compare
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8801 |
cf56e8a to
b286e88
Compare
Simplifying an inlined body could inline the functions it calls without limit, so a chain of small functions was inlined in full into every function. Stop after two nested levels; @inline functions are exempt. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Christoph Knittel <ck@cca.io>
b286e88 to
1bd640b
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LFDffswgCJNtcpW4vKw6et Signed-off-by: Christoph Knittel <ck@cca.io>
1bd640b to
b2a9eb0
Compare
|
@cristianoc For the examples/tests in the compiler repo, no output diff is observed with Still, I am open to a less strict max, what value would you suggest? |
Not sure. So far it points to removing the concept entirely. |
cristianoc
left a comment
There was a problem hiding this comment.
Will let you deal with tradeoffs and decide which way to go.
It is a safety bound against quadratic blowup; 4 leaves more room for nested inlining and still produces the same output for existing code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LFDffswgCJNtcpW4vKw6et Signed-off-by: Christoph Knittel <ck@cca.io>
The arguments were constants, so the calls were evaluated at compile time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LFDffswgCJNtcpW4vKw6et Signed-off-by: Christoph Knittel <ck@cca.io>
|
Decided to go ahead with a limit of 4. With 4, the 4,000-function chain compiles in 0.35s (0.29s with 2, 33s without a limit). |
When
Lam_pass_remove_aliasinlines a call to a small function, it simplifies the inlined body again, which can inline the functions that body calls, and so on. Each step passes the size check, because it only looks at the callee's original body, but nothing bounded the nesting. In a chain of small functions that each call the previous one, every function got the whole chain inlined:So both the generated code and compile time grew quadratically with the length of the chain.
Nested inlining now stops at depth 4; beyond that the compiler emits a call. Functions marked
@inlineare exempt, and functor inlining is unchanged.The limit is a safety bound against this blowup, not a code-size optimization. I compiled all test sources, Belt and the benchmark inputs (662 files) with limits 1 to 4:
4 leaves more room than needed for existing code while keeping the worst case linear.
New test:
inline_chain_test.reschecks a chain of small functions and a chain of@inlinefunctions at runtime. Its checked-in JS pins that the former stops inlining after four levels and the latter is still fully inlined.🤖 Generated with Claude Code
https://claude.ai/code/session_01LFDffswgCJNtcpW4vKw6et