-
Notifications
You must be signed in to change notification settings - Fork 46.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…ecls ghstack-source-id: da8ae28a969cd027c076e36a9a815665a3af5754 Pull Request resolved: #31066
…n decls ghstack-source-id: c0a4b9548787365d7183a02ed26be88101a234cb Pull Request resolved: #31066
…n decls ghstack-source-id: 32e551e45a9d48085dd2a7ab3cc5efc112808900 Pull Request resolved: #31066
…n decls ghstack-source-id: 485a44f7203083d0bdff4eaf042e71d30a0a7897 Pull Request resolved: #31066
temporaries: actuallyEvaluatedTemporaries, | ||
knownImmutableIdentifiers, | ||
hoistableFromOptionals, | ||
registry, | ||
); | ||
nestedFnImmutableContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we could technically reuse knownImmutableIdentifiers
for non-mutable captured identifiers. This is a bit more explicit
const maybeNonNull = getMaybeNonNullInInstruction(instr.value, context); | ||
if ( | ||
maybeNonNull != null && | ||
isImmutableAtInstr(maybeNonNull.fullPath.identifier, instr.id, context) | ||
) { | ||
assumedNonNullObjects.add(maybeNonNull); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just code movement. All mutability check logic moved to isImmutableAtInstr
because it got more complex ("if we're within a nested function, check if the base identifier is in nestedFnImmutableContext
, otherwise do the scope / mutable range check")
if ( | ||
instr.value.kind === 'FunctionExpression' && | ||
!fn.env.config.enableTreatFunctionDepsAsConditional | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core logic for collecting hoistable paths in inner functions
if ($[0] !== shouldReadA || $[1] !== a.b.c) { | ||
t1 = ( | ||
<Stringify | ||
fn={() => { | ||
if (shouldReadA) { | ||
return a.b.c; | ||
} | ||
return null; | ||
}} | ||
shouldInvokeFns={true} | ||
/> | ||
); | ||
$[0] = shouldReadA; | ||
$[1] = a.b.c; | ||
$[2] = t1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the propagate-hir-fork
version of this fixture (correctly) only adds a
as a dependency
local = $[1]; | ||
} | ||
let t1; | ||
if ($[2] !== shouldReadA || $[3] !== local) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional dependency within a function is truncated down to non-null load (e.g. local
)
if ($[0] !== shouldReadA || $[1] !== a) { | ||
t1 = ( | ||
<Stringify | ||
fn={() => { | ||
if (shouldReadA) { | ||
return a.b.c; | ||
} | ||
return null; | ||
}} | ||
shouldInvokeFns={true} | ||
/> | ||
); | ||
$[0] = shouldReadA; | ||
$[1] = a; | ||
$[2] = t1; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditional dependency within a function is truncated down to non-null load (e.g. a
)
if ($[0] !== a.b.c) { | ||
t0 = () => () => ({ value: a.b.c }); | ||
$[0] = a.b.c; | ||
$[1] = t0; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unconditional dependency within a nested function is retained (a.b.c
)
const $ = _c(2); | ||
let t0; | ||
if ($[0] !== a.b) { | ||
t0 = <Stringify fn={() => a.b?.c.d?.e} shouldInvokeFns={true} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a todo because we're still adding dependencies using LoweredFunction.dependencies
.
…n decls Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their *entry block*. Note that this only replaces the *hoisting* part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see [todo-infer-function-uncond-optionals-hoisted.tsx](https://github.com/facebook/react/blob/576f3c0aa898cb99da1b7bf15317756e25c13708/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx)) See newly added test fixtures for details ghstack-source-id: 41ef49d1312d4f1b6229bcd649e0a707aaca6160 Pull Request resolved: #31066
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, mostly makes sense but i'm not sure about saying that because a function accesses eg a.b.c
, that we can promote this to a non-nullable / hoistable expression. There's no guarantee the function will ever be called. OTOH if we knew the function was called, then it would be safe to make this assumption.
Yeah that's fair. One thing we could do is take the hoistable deps / paths of a function to be Intersect(hoistables in function definition site, ...hoistables in every known callsite). For functions that don't end up being called though (i.e. only passed around or captured by other non-called functions), this would mean that we don't infer anything to be hoistable. My assumptions were that a function should be type-safe to call for all post-dominators of the function definition site. I think this lines up with flow and typescript's modeling as well. // e.g. I believe this case should be rare because it produces a type error
const arr = getArrayOrNull();
const fn = () => arr.length;
if (arr != null) {
return fn();
}
// ... |
I'm not sure about the conclusion here for functions that aren't known to be called. Generally functions are accessing variables in local scope that are also referenced in other places during render, which would provde that those values are hoistable. So intersect(hoistable-in-function, hoistable-globally) seems like it would still let us get pretty good function deps and be fully safe. Can we try this? |
…n decls Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their *entry block*. Note that this only replaces the *hoisting* part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see [todo-infer-function-uncond-optionals-hoisted.tsx](https://github.com/facebook/react/blob/576f3c0aa898cb99da1b7bf15317756e25c13708/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx)) See newly added test fixtures for details ghstack-source-id: 1a856d335c986e5f73e1e1f809f592eac83f5cb2 Pull Request resolved: #31066
Chatted on VC, I see what you're getting at. Basically, type systems check functions based on where they're defined, not where they're potentially called, so developers generally have to add null-checks in functions to satisfy the type-checker. If they don't have a null-check, it's a good indication the value is not just non-nullable in practice, but a non-nullable type. Let's proceed as planned! |
Thanks for the quick review! Updated PR summary to include internal snapshot comparison + 2.5% statistic |
…n decls Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their *entry block*. Note that this only replaces the *hoisting* part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see [todo-infer-function-uncond-optionals-hoisted.tsx](https://github.com/facebook/react/blob/576f3c0aa898cb99da1b7bf15317756e25c13708/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.tsx)) See newly added test fixtures for details Update: Note that toggling `enableTreatFunctionDepsAsConditional` makes a non-trivial impact on granularity of inferred deps (i.e. we find that function declarations uniquely identify some paths as hoistable). Snapshot comparison of internal code shows ~2.5% of files get worse dependencies ([internal link](https://www.internalfb.com/phabricator/paste/view/P1625792186)) ghstack-source-id: 778ab5494a655875bc45a35225d41a95e202ca5b Pull Request resolved: #31066
…ting (#31032) Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #31066 * __->__ #31032 Prior to this PR, we check whether the property load source (e.g. the evaluation of `<base>` in `<base>.property`) is mutable + scoped to determine whether the property load itself is eligible for hoisting. This changes to check the base identifier of the load. - This is needed for the next PR #31066. We want to evaluate whether the base identifier is mutable within the context of the *outermost function*. This is because all LoadLocals and PropertyLoads within a nested function declaration have mutable-ranges within the context of the function, but the base identifier is a context variable. - A side effect is that we no longer infer loads from props / other function arguments as mutable in edge cases (e.g. props escaping out of try-blocks or being assigned to context variables)
…rtyLoads from function decls
d57086e
to
e0701a3
Compare
Stack from ghstack (oldest at bottom):
Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their entry block.
Note that this only replaces the hoisting part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see todo-infer-function-uncond-optionals-hoisted.tsx)
See newly added test fixtures for details
Update: Note that toggling
enableTreatFunctionDepsAsConditional
makes a non-trivial impact on granularity of inferred deps (i.e. we find that function declarations uniquely identify some paths as hoistable). Snapshot comparison of internal code shows ~2.5% of files get worse dependencies (internal link)