paulk-asert commented on PR #3230:
URL: https://github.com/apache/groovy/pull/3230#issuecomment-6672623020
Thanks, this makes a real difference. I compiled a generated 17,504-line
`@CompileStatic` class in a fresh JVM (`FileSystemCompiler`,
`-XX:+UseSerialGC`), comparing this PR with its parent (ee802a1814):
| `-Xmx` | before | this PR |
|---|---|---|
| 256m | OOM | OK |
| 320m | OOM | OK |
| 384m | OOM | OK |
| 448m | OK (15.0s) | OK (6.3s) |
| 512m | OK (9.1s) | OK (5.5s) |
One concern: removing `_PARAMETER_CONTEXT` by walking the finished AST can
miss parameters, and one missed parameter keeps the whole parse tree reachable.
I probed 17 shapes; two still keep a context after a full compile:
- closure parameters in an instance initializer block: `class C { def r; { r
= { int x -> x }(1) } }`
- closure parameters in a default parameter value: `def m(f = { int x -> x
}) { f(1) }`
Adding one such initializer, `{ q = { int x -> x }(1) }`, to the generated
class brings it back to failing at 256m and 320m. It passes at 384m, after 34s,
mostly in GC.
Since the context is only read during the visit
(`transformRecordHeaderToProperties`), could it be kept by the builder rather
than on the `Parameter`? That removes the walk entirely:
```java
private final Map<Parameter, GroovyParserRuleContext> parameterContexts =
new IdentityHashMap<>();
private final Map<Parameter, ModifierManager> parameterModifierManagers =
new IdentityHashMap<>();
// processFormalParameter
parameterModifierManagers.put(parameter, modifierManager);
parameterContexts.put(parameter, ctx);
// transformRecordHeaderToProperties
FormalParameterContext parameterCtx = (FormalParameterContext)
parameterContexts.get(parameter);
ModifierManager parameterModifierManager =
parameterModifierManagers.get(parameter);
// buildAST, finally
releaseRecognizers();
parameterContexts.clear();
parameterModifierManagers.clear();
```
`releaseParameterContexts`, `releaseClassParameterContexts`,
`clearParameterContexts`, `PARAMETER_CONTEXT_RELEASER` and the two metadata
keys then go away: +11/−60 in `AstBuilder`. Moving the modifier manager too
drops the remaining link from every parameter to the `AstBuilder`.
With that change:
- all 17 shapes keep no context;
- the class with the initializer closure compiles at 256m;
- `Groovy12332`, the parser tests, `gls.*`, `groovy.transform.stc.*`,
`RecordTest` and `RecordSpecificationTest` pass (114 classes, 4,487 tests).
The parameter assertion in `Groovy12332` would then pass trivially. It might
be worth adding the initializer-closure case, or asserting that no
`_PARAMETER_CONTEXT` metadata remains anywhere.
Releasing the parser and lexer looks safe:
- `Antlr4ParserPlugin` doesn't use them after `buildAST()`;
- `getOriginalText` only runs during the visit;
- tokens keep their own character stream, so token text still works.
I'd also suggest this for 6.0.1, since GROOVY-12332 is a 6.0.0 regression
that breaks builds at Gradle's default heap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
