Remove unused scope from Walker
#1341
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `scope` info here is entirely unused in the Glimmer codebase. It may be used in some AST transforms, but this is fairly unlikely, since it has only been available since Ember 3.16. Profiling template compilation highlighted the recursive walking of the whole tree for locals in scope during syntax parsing as a major cost in CPU usage, so the hope is that removing this will help.
This was referenced Oct 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
scopeinfo here is entirely unused in the Glimmer codebase. It may be used in some AST transforms, but this is fairly unlikely, since it has only been available since Ember 3.16. Profiling template compilation highlighted the recursive walking of the whole tree for locals in scope during syntax parsing as a major cost in CPU usage, so the hope is that removing this will help.I also confirmed that this passes all of Ember's tests when integrating into Ember 3.28! 🎉
Here's the output of two runs (A and B) using the latest version of this branch on our repro repo. To summarize: this gets us back another ~8s closer to the baseline throughput.
[email protected][email protected][email protected]@glimmer/[email protected]@glimmer/[email protected]@glimmer/compiler@master@glimmer/compiler@experiment(this PR)Note: There is something slightly odd happening – I suspect a quirk of the benchmark or of my machine setup, but haven't found what's causing it yet – where the 3.28 version will occasionally get "stuck" and have these blowup times. You can see that run A it showed up with a ~48s time on the
@glimmer/[email protected]variant, and on run B it showed up on the[email protected]variant.