compiler: Known hooks/nonescaping scopes dont count as pruned #29820
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
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This was referenced Jun 7, 2024
josephsavona
added a commit
that referenced
this pull request
Jun 8, 2024
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. ghstack-source-id: 75559d25ab3d3af6a330d91e69e4331006aa97c3 Pull Request resolved: #29820
facebook-github-bot
added
the
React Core Team
josephsavona
commented
Jun 8, 2024
Comment on lines
+28
to
+35
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
| t0 = ( | ||
| <div | ||
| className={stylex( | ||
| flags.feature("feature-name") ? styles.featureNameStyle : null, | ||
| )} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
This output matches what we have on main today. Whereas with the previous PR in the stack the output was:
const t0 = flags.feature("feature-name") ? styles.featureNameStyle : null;
let t1;
if ($[0] !== t0) {
t1 = stylex(t0);
$[0] = t0;
$[1] = t1;
} else {
t1 = $[1];
}
let t2;
if ($[2] !== t1) {
t2 = <div className={t1} />;
$[2] = t1;
$[3] = t2;
} else {
t2 = $[3];
}
return t2;Way more code and 4 cache slots instead of 1.
|
Comparing: a0a435d...8241eec Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
josephsavona
commented
Jun 8, 2024
| @@ -53,12 +53,11 @@ function Component(props) { | |||
| x = 1; | |||
| } | |||
| let t0; | |||
| if ($[0] !== x) { | |||
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | |||
There was a problem hiding this comment.
x is non-escaping (and therefore counted as pruned) and non-reactive. before this PR, we would have promoted x to reactive because of the pruned+nonreactive combination — now it doesn't count as pruned so we revert back to the output from main
…ned" There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 8, 2024
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. ghstack-source-id: b7c36b71aa44a7e4952670b5121c15fe9c96b123 Pull Request resolved: #29820
…ned" There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Jun 10, 2024
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. ghstack-source-id: 80610ddafad65eb837d0037e2692dd74bc548088 Pull Request resolved: #29820
josephsavona
commented
Jun 10, 2024
| @@ -331,29 +331,8 @@ class CountMemoBlockVisitor extends ReactiveFunctionVisitor<void> { | |||
| scopeBlock: PrunedReactiveScopeBlock, | |||
| state: void | |||
| ): void { | |||
| let isHookOnlyMemoBlock = false; | |||
There was a problem hiding this comment.
this logic is now in FlattenScopesWithHooksOrUse, and also takes account of use() and not just hooks
josephsavona
added a commit
that referenced
this pull request
Jun 10, 2024
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope: * Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them. * Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case. I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement. ghstack-source-id: 80610ddafad65eb837d0037e2692dd74bc548088 Pull Request resolved: #29820
This was referenced Jul 25, 2024
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.
Stack from ghstack (oldest at bottom):
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope:
I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement.