Fix nested lambda inference memory growth #5206 - #5207
Conversation
871baa6 to
742245e
Compare
742245e to
7b1dd77
Compare
Link nested speculative copies to their canonical source lambdas so parameterless lambda chains reuse existing per-target inference results without sharing parameter bindings or parser state. Fixes eclipse-jdt#5206 Signed-off-by: François Martin <f.martin@fastmail.com>
7b1dd77 to
97e0045
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a heap-exhaustion scenario in ECJ speculative overload resolution involving deeply nested lambdas by ensuring nested speculative lambda copies remain linked to their canonical source lambdas and can reuse per-target inference caches when safe (no parameters in the enclosing lambda chain). It also adds a focused regression suite to prevent reintroducing the combinatorial memory growth.
Changes:
- Update
LambdaExpression.copy()to align source and copied nested lambdas, linking eligible nested copies to canonical source lambdas and sharingcopiesPerTargetTypecaches when the enclosing lambda chain is parameterless. - Add a new regression test (
NestedLambdaInferenceTest) that generates deep nested overload/lambda chains (generic + concrete variants). - Register the new regression test in the compiler regression test suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/TestAll.java | Adds the new regression test class to the standard test suite. |
| org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NestedLambdaInferenceTest.java | New regression coverage for nested speculative overload resolution without heap blowups. |
| org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/LambdaExpression.java | Links nested speculative lambda copies to canonical sources and shares inference caches where safe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@stephan-herrmann - given this involves inference, I am assuming you will want to be the first reviewer, I have also signed up to review FYI |
stephan-herrmann
left a comment
There was a problem hiding this comment.
Some questions after a first scan of changes:
- could we have some tests that demonstrate the interleaving of lambdas with and without parameters? There is some complexity in this, which I'd like see live in action.
- would it make sense to add a shortcut such that "normal" scenarios could continue to work the "old" way?
Replace the traversal-index root check with an identity check, document context-local cache ownership below parameterized lambdas, and cover interleaved parameterized and parameterless lambdas. Signed-off-by: François Martin <f.martin@fastmail.com>
stephan-herrmann
left a comment
There was a problem hiding this comment.
I'm starting to understand the strategy 😄
I suggest to start the next round by looking at my comment in method collectLambdas(). If my idea there is sound, then some of the other comments might become obsolete.
| LambdaExpression copiedLambda = copiedLambdas.get(i).lambda(); | ||
| if (sourceLambda.sourceStart != copiedLambda.sourceStart || sourceLambda.sourceEnd != copiedLambda.sourceEnd) | ||
| throw new CopyFailureException(); | ||
| if (!source.cacheShareable()) { |
There was a problem hiding this comment.
At first I read this method name as a command to cache anything that is shareable.
Perhaps isCacheSharable is a clearer name for this record component?
There was a problem hiding this comment.
Agreed. The pruning change removes CollectedLambda, so there is no component left to rename. collectLambdas() now returns plain LambdaExpression values.
Addressed in b25d5f0.
| if (lambda.arguments.length > 0) | ||
| this.parameterizedLambdaDepth++; | ||
| lambdas.add(new CollectedLambda(lambda, this.parameterizedLambdaDepth == 0)); |
There was a problem hiding this comment.
This makes every lambda with parameters not-sharing. Moving the conditional increment below the addition to lambdas doesn't seem to break any tests.
So, what is the exact condition preventing caching? Having parameters, or: having parameters of an outer lambda in scope?
There was a problem hiding this comment.
Another way to avoid messing with non-sharable copies might be to just prune the traversal when a lambda has parameters (by returning false after handling the current lambda). That way shareInferenceCaches() wouldn't even see any non-sharing lambdas. WDYT? Wouldn't this allow you to simplify the code some more?
There was a problem hiding this comment.
A lambda can have parameters and still share its own cache. When ECJ creates a cache entry, it resolves a new copy of that lambda. The cache entry owns the bindings for its parameters.
The problem starts below that lambda. A nested lambda may use a parameter from the enclosing lambda. Its resolved references then belong to one specific copy of the enclosing lambda, so its cache must stay with that copy.
For example:
left -> produce(() -> left)left -> ... can share its cache. The cache entry owns its binding for left. The inner () -> left must stay local because it uses the left binding from one particular enclosing copy.
I changed the visitor as you suggested:
lambdas.add(lambda);
return lambda.arguments.length == 0;It records the current lambda and then stops before visiting its children when that lambda has parameters. shareInferenceCaches() therefore receives the lambda with parameters, but not any lambda inside it. This removes CollectedLambda, the depth counter, endVisit(), and the skip branch in the linking loop.
This is conservative: it stops below every lambda with parameters, even when its children do not use those parameters. This may share fewer caches than necessary, but it avoids sharing references to bindings from another enclosing copy.
The source lambda and its reparsed copy must contain the same collected lambdas in the same source order. The size check detects a different number of collected lambdas. The source-range check detects a different source position for a pair. After linking, every collected source/copy pair shares the original lambda's cache. Pruned descendants are not linked and keep their local caches.
The interleaving test also contains a parameterless sibling after the parameterized branch, so this structure is covered.
Addressed in b25d5f0.
| if (originalLambda.copiesPerTargetType == null) | ||
| originalLambda.copiesPerTargetType = new HashMap<>(); |
There was a problem hiding this comment.
Now the responsibility to instantiate that HashMap is split between cachedResolvedCopy() and this current location.
Would it make sense to assign the sole responsibility to the current method, so cachedResolvedCopy() can safely assume an existing map instance?
Given that linking copiesPerTargetType is the main purpose of this method, maybe it should take full responsibility?
There was a problem hiding this comment.
Agreed, done. shareInferenceCaches() now creates the map, and I removed the fallback from cachedResolvedCopy().
put() is reached only after copy() returned a non-null value and the copy resolved successfully. A non-null result from copy() means that shareInferenceCaches() completed. That method always collects the root and assigns the original lambda's map, so this.copiesPerTargetType is non-null before put() is reached.
If parsing or cache linking fails, control does not reach put().
Addressed in b25d5f0.
|
Thanks, your pruning idea works and made the code much simpler.
I added the details in the inline replies. After this change, all 54 I also updated the pull request description to match the new approach. |
Collect a lambda with parameters and then stop below it. Nested lambdas may use that lambda's parameter bindings, so their caches must stay local to the enclosing copy. The lambda itself can share its cache because each cached copy has its own parameter bindings. Remove the cache eligibility record and depth counter. Let shareInferenceCaches() create copiesPerTargetType in one place. Signed-off-by: François Martin <f.martin@fastmail.com>
dd2de41 to
b25d5f0
Compare
What it does
Fixes #5206.
During speculative overload resolution, ECJ tries each overload candidate without committing to one. It reparses the outer lambda to create a separate copy for that candidate. Each source lambda stores inference results for the target functional-interface types against which ECJ has already tested it. The lambda from which a copy was made is its original lambda.
The outer speculative copy kept that source link, but nested lambdas inside the copy were treated as new originals. They therefore did not reuse the existing per-target results from their original lambdas. Repeating inference for every candidate at every nested level caused combinatorial memory growth and exhausted the heap.
The change links nested speculative copies to their original lambdas. On each branch, it shares the per-target inference cache down to and including the first lambda with parameters. A cache entry is a resolved copy of that lambda and owns its parameter bindings. Nested lambdas may use those bindings, so traversal stops below it and their caches stay local to the enclosing copy. No global cache or shared parser state is introduced.
The source lambda and the copy created by parsing the source again are traversed in the same source order. Each traversal records the current lambda, then stops below it when it has parameters.
shareInferenceCaches()therefore receives only lambdas allowed by this parameter-binding rule, so no separate eligibility flag is needed.The independently designed regression suite has three tests. Two generate a generic and a concrete source. Each source contains 24 nested method calls and 23 parameterless lambdas, alternates between
routeandselect, provides five overload candidates for each selector, and ends at aWork<String>field. The two variants exercise the original missing cache reuse with different inference pressure.The third test compiles and runs code that mixes lambdas with and without parameters. It includes a nested lambda that reads an enclosing parameter and a separate parameterless sibling. There is no public API change.
The linked NestedLambdaGenerics.java and NestedLambdaNoGenerics.java files provide separate reference validation for the reported behavior; they are not copied into the regression suite.
How to test
792a9156c119d28063904c85726e486ec2fb2206, compiling each linked OpenJDK source separately with-25and a 1 GiB heap reproduces Nested lambda overload inference exhausts the heap #5206.OutOfMemoryErroror internal compiler error.NestedLambdaInferenceTestacross Java 8 through Java 25 compliance levels: 54 tests pass.GenericsRegressionTest_1_8together withNestedLambdaInferenceTest: 5,670 tests pass, including the parameterized-lambda regressiontestBug496578.parser/TestAll,regression/TestAll, andeval/TestAll): 263,722 tests pass with no failures, errors, or skipped tests.Author checklist