Codegen: Inline the target of Function nodes in their js.Closures.
#5081
Conversation
f2c03d6 to
bb61516
Compare
| def gen(tree: Tree): Unit = { | ||
| tree match { | ||
| case EmptyTree => () | ||
| case Template(_, _, body) => body foreach gen |
There was a problem hiding this comment.
Just to double check: This was dead code?
There was a problem hiding this comment.
Not strictly dead code, but it would only happen for the top-level "recursive" call (we cannot have a Template inside a Temp
8000
late, and would always happen for the top-level call (the input is statically typed as Template, so we know that). The new code directly iterates over impl.body instead.
| def gen(tree: Tree): Unit = { | ||
| tree match { | ||
| case EmptyTree => () | ||
| case Template(_, _, body) => body foreach gen |
There was a problem hiding this comment.
Same here: This was dead-code?
| /* Declare each method param as a VarDef, initialized to the corresponding arg. | ||
| * In practice, all the args are `This` nodes of `VarRef` nodes, so the | ||
| * optimizer will alias those VarDefs away. | ||
| * We do it in two steps because we have different Symbols, hence different |
There was a problem hiding this comment.
| * We do it in two steps because we have different Symbols, hence different | |
| * We do this because we have different Symbols, hence different |
WDYT? The current wording confused me.
| val patchedBody = | ||
| js.Block(paramsLocals :+ ensureResultBoxed(body, target)) | ||
| /* Declare each method param as a VarDef, initialized to the corresponding arg. | ||
| * In practice, all the args are `This` nodes of `VarRef` nodes, so the |
There was a problem hiding this comment.
| * In practice, all the args are `This` nodes of `VarRef` nodes, so the | |
| * In practice, all the args are `This` nodes or `VarRef` nodes, so the |
| */ | ||
| val formalArgs = paramTrees.map(p => genParamDef(p.symbol)) | ||
| val (patchedFormalArgs, paramsLocals) = | ||
| patchFunParamsWithBoxes(target, formalArgs, useParamsBeforeLambdaLift = true) |
There was a problem hiding this comment.
It is a bit ugly IMO that we introduce another set of VarDefs here for unboxing.
- Nevertheless, this change is clearly an improvement, so I think we can live with a TODO.
- IIUC, this would change again with Introduce NewLambda to synthesize instances of SAM types. #5003 , so maybe it makes sense to fix it after that only?
There was a problem hiding this comment.
This will definitely change again with #5003. That said per se the additional VarDefs will still be there, since types don't always align.
The changes in this PR are neutral in terms of number of assignments that need to be processed by the optimizer: we traded Bindings for ParamDefs in a method call for Bindings to local VarDefs.
We can look into improving this a bit further after #5003 if we think it's worth it.
bb61516 to
6737d97
Compare
The body of `Function` nodes always has a simple shape that calls a helper method. We previously generated that call in the body of the `js.Closure`, and marked the target method `@inline` so that the optimizer would always inline it. Instead, we now directly "inline" it from the codegen, by generating the `js.MethodDef` right inside the `js.Closure` scope. As is, this does not change the generated code. However, it may speed up (cold) linker runs, since it will have less work to do. Notably, it performs two fewer knowledge queries to find and inline the target method. It also reduces the total amount of methods to manipulate in the incremental analysis. More importantly, this will be necessary later if we want to add support for `async/await` or `function*/yield`. Indeed, for those, we will need `await`/`yield` expressions to be lexically scoped in the body of their enclosing closure. That won't work if they are in the body of a separate helper method.
6737d97 to
0d16b42
Compare
The body of
Functionnodes always has a simple shape that calls a helper method. We previously generated that call in the body of thejs.Closure, and marked the target method@inlineso that the optimizer would always inline it.Instead, we now directly "inline" it from the codegen, by generating the
js.MethodDefright inside thejs.Closurescope.As is, this does not change the generated code. However, it may speed up (cold) linker runs, since it will have less work to do. Notably, it performs two fewer knowledge queries to find and inline the target method. It also reduces the total amount of methods to manipulate in the incremental analysis.
More importantly, this will be necessary later if we want to add support for
async/awaitorfunction*/yield. Indeed, for those, we will needawait/yieldexpressions to be lexically scoped in the body of their enclosing closure. That won't work if they are in the body of a separate helper method.