[compiler] Add lowerContextAccess pass #30548

gsathya · 2024-07-31T13:35:37Z

Stack from ghstack (oldest at bottom):

This is only for internal profiling, not intended to ship.

This pass is intended to be used with #30407.

This pass synthesizes selector functions by collecting immediately
destructured context acesses. We bailout for other types of context
access.

This pass lowers context access to use a selector function by passing
the synthesized selector function as the second argument.

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

vercel · 2024-07-31T13:35:39Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 7, 2024 3:01pm

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 3dde92d Pull Request resolved: #30548

gsathya · 2024-07-31T13:39:14Z

I ended up synthesizing new functions in the IR as it works well with the outlining infra, but it does add a fair amount of code. If anyone feels strongly, I'm happy to move this to codegen and emit babel AST directly.

There's still a bunch of follow ups after this lands:

Should we emit code to check gk?
Should we compile to a different function rather than useContext?

cc @jackpope

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 0cefe6c Pull Request resolved: #30548

jackpope · 2024-07-31T15:59:16Z

Should we emit code to check gk?
Should we compile to a different function rather than useContext?

Assuming we can enable the flag internally only, we can compile to an intermediate hook defined on WWW which can handle the GK/QE. Something like unstable_useContextWithBailoutExperiment. Example: D60462596

josephsavona

This is awesome, super exciting to see this come together. I'm really curious about the results of the experiment.

In terms of the implementation, see comments. The key thing to address is the ArrayPattern case - make it a todo or add tests - but also using createTemporaryPlace() will reduce the boilerplate by quite a bit.

compiler/packages/babel-plugin-react-compiler/src/Optimization/LowerContextAccess.ts

josephsavona · 2024-07-31T18:09:05Z

I'm happy to move this to codegen and emit babel AST directly

I like the overall approach here of using HIR to construct the function.

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: f01152c Pull Request resolved: #30548

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 6a8d58e Pull Request resolved: #30548

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 45b59f0 Pull Request resolved: #30548

josephsavona

Thanks for the updates and extra tests, this looks great! See comment below, i could see landing this with missing fixtures temporarily, but it seems like the next steps are fairly straightforward and could also happen directly in this PR (convert the destructure to an ArrayPattern, make the fixtures run in sprout).

josephsavona · 2024-08-02T19:46:07Z

...t-compiler/src/__tests__/fixtures/compiler/todo.lower-context-access-property-load.expect.md

+```
+
+### Eval output
+(kind: exception) Fixture not implemented


missing fixture

josephsavona · 2024-08-02T19:46:12Z

...ler/src/__tests__/fixtures/compiler/todo.lower-context-access-nested-destructuring.expect.md

+```
+
+### Eval output
+(kind: exception) Fixture not implemented


josephsavona · 2024-08-02T19:50:19Z

...lugin-react-compiler/src/__tests__/fixtures/compiler/lower-context-selector-simple.expect.md

+  const { foo, bar } = useContext(MyContext, _temp);
+  let t0;
+  if ($[0] !== foo || $[1] !== bar) {
+    t0 = <Bar foo={foo} bar={bar} />;
+    $[0] = foo;
+    $[1] = bar;
+    $[2] = t0;
+  } else {
+    t0 = $[2];
+  }
+  return t0;
+}
+function _temp(t0) {
+  return [t0.foo, t0.bar];
+}


How are you thinking about sequencing the next steps here? Asking because the generated code doesn't seem like it could work yet: the temp function returns an array, but the context value is destructured into an object and there's nothing that tells us how to map the array into the object.

Seems like we'd need to convert the destructuring into an ArrayPattern to match? Also, if we take Jack's suggestion of compiling into a custom useContextSelector-style hook, then we can implement a basic version of that in shared-runtime and this could be an end-to-end test with sprout.

The return value of the selector isn't returned from the useContext call. The runtime simply compares the return values from the selector to determine if the context is dirty. If the values are different, then it returns the context object from useContext call, not the values from the selector function.

Ideally we'd just compile the selector function with Forget and compare the result, rather than iterate over an array, but that's optimisation for the future.

ahhhhh got it, makes sense

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 8a1ba69 Pull Request resolved: #30548

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

gsathya · 2024-08-06T15:31:10Z

Lowering to a different function call implemented here: #30612

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. [ghstack-poisoned]

*This is only for internal profiling, not intended to ship.* This pass is intended to be used with #30407. This pass synthesizes selector functions by collecting immediately destructured context acesses. We bailout for other types of context access. This pass lowers context access to use a selector function by passing the synthesized selector function as the second argument. ghstack-source-id: 92d0f6f Pull Request resolved: #30548

gsathya mentioned this pull request Jul 31, 2024

[compiler] Simplify FunctionExpression node #30544

Merged

This was referenced Jul 31, 2024

[compiler] Refactor makeTemporary outside HIRBuilder #30545

Merged

[compiler] Add typing for useContext hook #30546

Merged

[compiler] Add flag for lowering context access #30547

Merged

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 31, 2024

vercel bot deployed to Preview July 31, 2024 13:40 View deployment

vercel bot deployed to Preview July 31, 2024 13:47 View deployment

josephsavona requested changes Jul 31, 2024

View reviewed changes

gsathya requested a review from josephsavona August 2, 2024 18:12

vercel bot deployed to Preview August 2, 2024 18:21 View deployment

josephsavona reviewed Aug 2, 2024

View reviewed changes

gsathya added 2 commits August 6, 2024 15:17

vercel bot deployed to Preview August 6, 2024 14:24 View deployment

gsathya requested a review from josephsavona August 6, 2024 14:29

gsathya mentioned this pull request Aug 6, 2024

[compiler] Rewrite useContext callee #30612

Merged

vercel bot deployed to Preview August 6, 2024 15:39 View deployment

josephsavona approved these changes Aug 6, 2024

View reviewed changes

vercel bot deployed to Preview August 6, 2024 16:17 View deployment

vercel bot deployed to Preview August 6, 2024 16:21 View deployment

vercel bot deployed to Preview August 7, 2024 15:01 View deployment

gsathya merged commit fec8e95 into gh/gsathya/14/base Aug 7, 2024
19 checks passed

gsathya deleted the gh/gsathya/14/head branch August 7, 2024 15:30

eps1lon mentioned this pull request Aug 14, 2024

Update React from 187dd6a7-20240806 to 49496d49-20240814 vercel/next.js#68898

Merged

[compiler] Add lowerContextAccess pass #30548

[compiler] Add lowerContextAccess pass #30548

Uh oh!

Conversation

gsathya commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsathya commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackpope commented Jul 31, 2024

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephsavona commented Jul 31, 2024

Uh oh!

josephsavona left a comment

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

gsathya Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsavona Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

gsathya commented Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

gsathya commented Jul 31, 2024 •

edited

Loading

vercel bot commented Jul 31, 2024 •

edited

Loading

gsathya commented Jul 31, 2024 •

edited

Loading

gsathya Aug 6, 2024 •

edited

Loading